[m-rev.] for review: Allow external files to be included in foreign_decl and foreign_code.

[Paul Bone]

On Thu, Mar 13, 2014 at 03:28:17PM +1100, Peter Wang wrote:
> 
> compiler/file_names.m:
> 	Add a convenience to get a module's source file name.
> 

Missing word?

> 	Add make_include_file_path.
> 
> 	Reorder a predicate.

reorder it's contents or it's position with respect to other predicates?

> diff --git a/compiler/elds_to_erlang.m b/compiler/elds_to_erlang.m
> index c91151b..3a9b808 100644
> --- a/compiler/elds_to_erlang.m
> +++ b/compiler/elds_to_erlang.m
> @@ -29,7 +30,8 @@
>      % and exported foreign_decls to the corresponding .hrl file.
>      % The file names are determined by the module name.
>      %
> -:- pred output_elds(module_info::in, elds::in, io::di, io::uo) is det.
> +:- pred output_elds(module_info::in, elds::in, bool::out, io::di, io::uo)
> +    is det.

Would it be better to create a new type to represent the meaning of the bool
output.  I guess this is for success/failure:

    :- type success
        --->    success
        ;       failure.

Although this is equivalent to bool it allows someone reading the
code to clearly see if true means success or if true meas failure - by not
using true/false or yes/no at all.

> diff --git a/compiler/make.module_dep_file.m b/compiler/make.module_dep_file.m
> index bc4ea81..8c2bb40 100644
> --- a/compiler/make.module_dep_file.m
> +++ b/compiler/make.module_dep_file.m
> @@ -63,6 +63,22 @@
>  :- import_module term.
>  :- import_module term_io.
>  
> +    % The version 1 module_dep file format is the same as version 2 except that
> +    % it does not include a list of files included by `pragma foreign_decl' and
> +    % `pragma foreign_code'. We continue to write version 1 files when
> +    % possible.
> +    %

Why bother?  I don't think that there's a need to share .dep files between
Mercury versions, so it's be simpler to always use version 2 files.

> @@ -397,12 +450,42 @@ read_module_dependencies_2(Globals, RebuildModuleDeps, SearchDirs, ModuleName,
>          SearchResult, !IO),
>      (
>          SearchResult = ok(ModuleDir),
> -        parser.read_term(ImportsTermResult, !IO),
> +        parser.read_term(TermResult, !IO),
>          io.set_input_stream(OldInputStream, ModuleDepStream, !IO),
>          io.close_input(ModuleDepStream, !IO),
>          (
> -            ImportsTermResult = term(_, ImportsTerm),
> -            ImportsTerm = term.functor(term.atom("module"), ModuleArgs, _),
> +            TermResult = term(_, Term),
> +            read_module_dependencies_3(Globals, SearchDirs, ModuleName,
> +                ModuleDir, Term, Result, !Info, !IO)
> +        ;
> +            TermResult = eof,
> +            Result = error("unexpected eof")
> +        ;
> +            TermResult = error(ParseError, _),
> +            Result = error("parse error: " ++ ParseError)
> +        ),
> +        (
> +            Result = ok
> +        ;
> +            Result = error(Msg),
> +            read_module_dependencies_remake(Globals, RebuildModuleDeps,
> +                ModuleName, Msg, !Info, !IO)
> +        )
> +    ;
> +        SearchResult = error(_),
> +        % XXX should use the error message.
> +        read_module_dependencies_remake(Globals, RebuildModuleDeps, ModuleName,
> +            "couldn't find `.module_dep' file", !Info, !IO)
> +    ).

Did you forget this before committing?

> diff --git a/compiler/mercury_compile.m b/compiler/mercury_compile.m
> index 82626d3..1d51907 100644
> --- a/compiler/mercury_compile.m
> +++ b/compiler/mercury_compile.m
> @@ -1654,18 +1659,24 @@ mercury_compile_after_front_end(NestedSubModules, FindTimestampFiles,
>              % than stdout.
>              io.stdout_stream(Stdout, !IO),
>              output_x86_64_asm(Stdout, X86_64_Asm, !IO),
> +            Succeeded = yes,
>              ExtraObjFiles = []
>          ;
>              Target = target_erlang,
>              erlang_backend(!.HLDS, ELDS, !DumpInfo, !IO),
> -            elds_to_erlang(!.HLDS, ELDS, !IO),
> +            elds_to_erlang(!.HLDS, ELDS, Succeeded, !IO),
>              ExtraObjFiles = []
>          ),
> +        (
> +            Succeeded = yes,
>              recompilation.usage.write_usage_file(!.HLDS, NestedSubModules,
>                  MaybeTimestamps, !IO),
>              FindTimestampFiles(ModuleName, TimestampFiles, !IO),
>              list.foldl(touch_datestamp(Globals), TimestampFiles, !IO)
>          ;
> +            Succeeded = no
> +        )

I assume this error is raised elsewhere.  I'm not saying that this is wrong,
just that it looks suspicious and a comment could re-assure the reader of
the context doesn't make it clear enough.

Also this should probably not be a bool but a new type.

> diff --git a/compiler/mercury_compile_mlds_back_end.m b/compiler/mercury_compile_mlds_back_end.m
> index 25776f2..609dca4 100644
> --- a/compiler/mercury_compile_mlds_back_end.m
> +++ b/compiler/mercury_compile_mlds_back_end.m
> @@ -38,13 +38,17 @@
>  :- pred maybe_mark_static_terms(bool::in, bool::in,
>      module_info::in, module_info::out, io::di, io::uo) is det.
>  
> -:- pred mlds_to_high_level_c(globals::in, mlds::in, io::di, io::uo) is det.
> +:- pred mlds_to_high_level_c(globals::in, mlds::in, bool::out,
> +    io::di, io::uo) is det.
>  
> -:- pred mlds_to_java(module_info::in, mlds::in, io::di, io::uo) is det.
> +:- pred mlds_to_java(module_info::in, mlds::in, bool::out,
> +    io::di, io::uo) is det.
>  
> -:- pred mlds_to_csharp(module_info::in, mlds::in, io::di, io::uo) is det.
> +:- pred mlds_to_csharp(module_info::in, mlds::in, bool::out,
> +    io::di, io::uo) is det.
>  
> -:- pred mlds_to_il_assembler(globals::in, mlds::in, io::di, io::uo) is det.
> +:- pred mlds_to_il_assembler(globals::in, mlds::in, bool::out,
> +    io::di, io::uo) is det.
>  

More bools used as result types.

> diff --git a/compiler/mlds_to_c.m b/compiler/mlds_to_c.m
> index 8fa8d8a..20ef4af 100644
> --- a/compiler/mlds_to_c.m
> +++ b/compiler/mlds_to_c.m
> @@ -31,11 +31,12 @@
>  :- import_module libs.globals.
>  :- import_module ml_backend.mlds.
>  
> +:- import_module bool.
>  :- import_module io.
>  
>  %-----------------------------------------------------------------------------%
>  
> -    % output_c_mlds(MLDS, Globals, Suffix):
> +    % output_c_mlds(MLDS, Globals, Suffix, Succeeded):
>      %
>      % Output C code the the appropriate C file and C declarations to the
>      % appropriate header file. The file names are determined by the module
> @@ -43,28 +44,28 @@
>      % for debugging dumps. For normal output, the suffix should be the empty
>      % string.)
>      %
> -:- pred output_c_mlds(mlds::in, globals::in, string::in, io::di, io::uo)
> -    is det.
> +:- pred output_c_mlds(mlds::in, globals::in, string::in, bool::out,
> +    io::di, io::uo) is det.

And here.

> diff --git a/compiler/mlds_to_cs.m b/compiler/mlds_to_cs.m
> index e8d2659..aa43ebd 100644
> --- a/compiler/mlds_to_cs.m
> +++ b/compiler/mlds_to_cs.m
> @@ -32,11 +32,13 @@
>  :- import_module hlds.hlds_module.
>  :- import_module ml_backend.mlds.
>  
> +:- import_module bool.
>  :- import_module io.
>  
>  %-----------------------------------------------------------------------------%
>  
> -:- pred output_csharp_mlds(module_info::in, mlds::in, io::di, io::uo) is det.
> +:- pred output_csharp_mlds(module_info::in, mlds::in, bool::out,
> +    io::di, io::uo) is det.
>  

And in this file.

> diff --git a/compiler/mlds_to_ilasm.m b/compiler/mlds_to_ilasm.m
> index e17ba5b..6a08de6 100644
> --- a/compiler/mlds_to_ilasm.m
> +++ b/compiler/mlds_to_ilasm.m
> @@ -21,13 +21,15 @@
>  :- import_module libs.globals.
>  :- import_module ml_backend.mlds.
>  
> +:- import_module bool.
>  :- import_module io.
>  
>  %-----------------------------------------------------------------------------%
>  
>      % Convert the MLDS to IL and write it to a file.
>      %
> -:- pred output_mlds_via_ilasm(globals::in, mlds::in, io::di, io::uo) is det.
> +:- pred output_mlds_via_ilasm(globals::in, mlds::in, bool::out,
> +    io::di, io::uo) is det.
>  

And here.

> diff --git a/compiler/mlds_to_java.m b/compiler/mlds_to_java.m
> index fcf3b90..867a64c 100644
> --- a/compiler/mlds_to_java.m
> +++ b/compiler/mlds_to_java.m
> @@ -5239,14 +5268,16 @@ indent_line(N, !IO) :-
>      string::in, prog_context::in, io::di, io::uo) is det.
>  
>  write_string_with_context_block(Info, Indent, Code, Context, !IO) :-
> -    indent_line(Info, marker_begin_block, mlds_make_context(Context),
> -        Indent, !IO),
> +    indent_line_prog_context(Info, marker_begin_block, Context, Indent, !IO),
>      io.write_string(Code, !IO),
>      io.nl(!IO),
> +    % The num_lines(Code) call is supposed to count the number of lines
> +    % occupied by Code in the source file. The result will be incorrect if
> +    % there were any escape sequences representing CR or LF characters --
> +    % they are expanded out in Code.
>      Context = context(File, Lines0),
>      ContextEnd = context(File, Lines0 + num_lines(Code)),
> -    indent_line(Info, marker_end_block, mlds_make_context(ContextEnd),
> -        Indent, !IO).
> +    indent_line_prog_context(Info, marker_end_block, ContextEnd, Indent, !IO).

I can look into this.  I was wondering the line number markers which are
used by mfilterjavac are compatible with including foreign code from
external files, it looks like they are because the context includes File.

> diff --git a/compiler/prog_io_typeclass.m b/compiler/prog_io_typeclass.m
> index 5d19908..4d383a2 100644
> --- a/compiler/prog_io_typeclass.m
> +++ b/compiler/prog_io_typeclass.m
> @@ -30,7 +30,7 @@
>  
>      % Parse a typeclass declaration.
>      %
> -:- pred parse_typeclass(module_name::in, varset::in, list(term)::in,
> +:- pred parse_typeclass(module_name::in,varset::in, list(term)::in,
>      prog_context::in, int::in, maybe1(item_typeclass_info)::out) is semidet.
>  
>      % Parse an instance declaration.

Whitespace

> diff --git a/compiler/write_deps_file.m b/compiler/write_deps_file.m
> index bc7e0f9..33b6e77 100644
> --- a/compiler/write_deps_file.m
> +++ b/compiler/write_deps_file.m
> @@ -36,6 +36,9 @@
>      % `.trans_opt' file may depend on.  This is set to `no' if the
>      % dependency list is not available.
>      %
> +    % XXX we do not yet write dependencies on files referenced by pragma
> +    % foreign_decl or pragma foreign_code
> +    %
>  :- pred write_dependency_file(globals::in, module_and_imports::in,
>      set(module_name)::in, maybe(list(module_name))::in, io::di, io::uo) is det.
> 

Does this mean that if my pragma says use the file "foo.c" that mmc doesn't
know if foo.c depends upon bar.c?  That seems okay to me.  If I
misunderstand please let me know.

> diff --git a/doc/reference_manual.texi b/doc/reference_manual.texi
> index a7a34b2..525aa26 100644
> --- a/doc/reference_manual.texi
> +++ b/doc/reference_manual.texi
> @@ -9522,6 +9522,7 @@ The University of Melbourne Mercury implementation supports the following
>  extensions to the Mercury language:
>  
>  @menu
> +* Foreign include files::       Including foreign code from external files.
>  * Fact tables::                 Support for very large tables of facts.
>  @c XXX STM
>  @c The documentation of STM is commented out because its support is
> @@ -9545,6 +9546,29 @@ extensions to the Mercury language:
>  @c     implementation-specific...
>  @c * Reserved tag::		Support for Herbrand constraint solvers.
>  
> + at node Foreign include files
> + at section Foreign include files
> +
> +Foreign declarations and code may be included from external files
> +using extensions of the @samp{pragma foreign_decl} and
> + at samp{pragma foreign_code} declarations:

I'd say "...using the @samp{pragma foreign_decl}...".  This might be an
implementation specific extension to the Mercury language, but I don't think
it needs to be explained as an extension to this syntax.

> +
> + at example
> +:- pragma foreign_decl("@var{Lang}", include_file("@var{Path}")).
> +:- pragma foreign_decl("@var{Lang}", local, include_file("@var{Path}")).
> +:- pragma foreign_code("@var{Lang}", include_file("@var{Path}")).
> + at end example
> +
> +These have the same effects as the standard forms except that the
> +contents of the file referenced by @var{Path} are included in place of
> +the string literal in the last argument, without further interpretation.
> + at var{Path} may be an absolute path to a file or a path relative to the
> +directory that contains the source file of the module containing the
> +declaration. The interpretation of the path is platform-dependent.

This is a little hard to parse and readers my interpret it to mean that Path
is either a file (absolute path to a file) or a directory (when relative).
I suggest:

    @var{Path} refers to the file to be included during compilation, it may
    either be absolute or relative to the directory that contains the source
    file of the module containing the declaration.


> diff --git a/tests/mmc_make/Mmakefile b/tests/mmc_make/Mmakefile
> index 7fe6bc6..00dc31d 100644
> --- a/tests/mmc_make/Mmakefile
> +++ b/tests/mmc_make/Mmakefile
> @@ -30,6 +32,12 @@ include $(TESTS_DIR)/Mmake.common
>  
>  complex_test.log: install_libs
>  
> +# Check that included files are identified as dependencies of the target code.
> +include_file2.runtest: include_file
> +	sleep 1 && touch inc/code.c inc/code.java inc/code.cs inc/code.erl
> +	$(MCM) --verbose-make include_file > include_file2.err 2>&1
> +	grep '^Making Mercury/.*/include_file[.]' include_file2.err
> +

s/MCM/MMC ?

Please document why this should "sleep 1".

> diff --git a/tests/mmc_make/include_file.m b/tests/mmc_make/include_file.m
> new file mode 100644
> index 0000000..26fc20d
> --- /dev/null
> +++ b/tests/mmc_make/include_file.m
> @@ -0,0 +1,64 @@
> +:- module include_file.
> +:- interface.
> +
> +:- import_module io.
> +
> +:- pred main(io::di, io::uo) is det.
> +
> +:- implementation.
> +
> +:- pragma foreign_decl("C", include_file("inc/decl.h")).
> +:- pragma foreign_code("C", include_file("inc/code.c")).

That's cool, I like the way this feature works.


Thanks Peter, the rest of the patch looks good.


-- 
Paul Bone