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

[Peter Wang]

On Sat, 15 Mar 2014 15:59:48 +1100, Paul Bone <paul at bone.id.au> wrote:
> 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?
> 

No, but I'll change it.

> > 	Add make_include_file_path.
> > 
> > 	Reorder a predicate.
> 
> reorder it's contents or it's position with respect to other predicates?
> 

The latter.

> > 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.
> 

Right, but the compiler already uses bool for success in quite a number
of places so I didn't want to introduce a new convention.  Not in this
commit anyway.

> > 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.

It ensures I didn't break parsing of version 1 files.  It may be very
slightly handy for developers jumping between older and newer versions
of the compiler (git bisect?).  It's not a great bother.

> > +        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?

Code movement; I didn't write that.

> > 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.

You mean?

    Succeeded = no
    % An error should have been reported earlier.

> > 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.

The other backends just reset the context to the line in the _target_
file at the end of the foreign code block, which is what you want.
The C and Erlang backends use the line number of the output stream as
tracked by the Mercury standard library.  The C# backend writes "#line
default" which the C# compiler understands.  Either would work for
mfilterjavac.

> > 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.
> 

That's true, but not what the comment refers to.

write_deps_file.m deals with mmake.  mmake wouldn't know that foo.m
includes foo_code.c, so if you edit foo_code.c then run mmake foo,
it would incorrectly assume that foo is already up-to-date.
The problem is fixed in the subsequent patch.

> > 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.

I'll change it to:

    Foreign declarations and code may be included from external files
    in @samp{pragma foreign_decl} and @samp{pragma foreign_code} declarations:

> 
> > +
> > + 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.

I'll change it to:

    @var{Path} may be an absolute path to a file, or a path to a file
    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 ?

MCM

> 
> Please document why this should "sleep 1".

Done.

> 
> Thanks Peter, the rest of the patch looks good.
> 

Thanks for the review.

Peter