[m-rev.] for post-commit review: make linking code more readable

[Julien Fischer]

On Sun, 19 Jan 2025 at 21:02, Zoltan Somogyi <zoltan.somogyi at runbox.com> wrote:
>
> For review by anyone. I am seeking ideas for better names
> for the two main predicates affected by this diff in compile_target_code.m.

Nothing springs to mind.

> I also noticed an annoying if minor inconsistency. Many predicates in the
> updated modules take both ProgressStream and Globals as arguments, but
> some take them in one order, and some take them in the other. Both are
> justifiable, but consistency would better. Any preferences?

Based on the rest of the compiler, it should be ProgressStream, then Globals.
(With ProgressStream being a predicate's first argument if possible.)

> Improve the readability of linking code.
>
> compiler/compile_target_code.m:
>     Give the two main predicates that do linking more descriptive names.
>     Expand the comment on one, and add a comment on the other.
>
>     Make one of these predicates take as input a list of module names,
>     not a list of file names. Previously, we converted module names to
>     file names, passed them to this predicate, which then converted them
>     *back* to module names. The new code, by avoiding unneeded
>     transformations, also avoids the possibility of callers passing
>     file names that did not start out as module names, which therefore
>     cannot be *meaningfully* converted back to module names.
>
>     Move the globals argument of a predicate to one of its usual places,
>     because the reason for its being supplied late does not apply anymore.
>
>     Consistently use a single variable names to describe linked_target_types;
>     some of the old variable names were misleading.
>
> compiler/make.module_target.m:
> compiler/make.program_target.m:
> compiler/make.top_level.m:
> compiler/mercury_compile_augment.m:
> compiler/mercury_compile_main.m:
>     Conform to the changes above.
>
> diff --git a/compiler/compile_target_code.m b/compiler/compile_target_code.m
> index db74c2474..94decec03 100644
> --- a/compiler/compile_target_code.m
> +++ b/compiler/compile_target_code.m
> @@ -84,18 +84,22 @@
>      module_name::in, list(module_name)::in, maybe(file_name)::out,
>      io::di, io::uo) is det.
>
> -    % link_module_list(ProgressStream, ModulesToLink, ExtraObjFiles,
> -    %   Globals, Specs, Succeeded, !IO):
> +    % link_modules_into_executable_or_shared_library(ProgressStream, Globals,
> +    %   ModulesToLink, ExtraObjFileNames, Specs, Succeeded, !IO):
>      %
> -    % The elements of ModulesToLink are the output of
> -    % `module_name_to_filename(ModuleName, "", no, ModuleToLink)'
> -    % for each module in the program.
> +    % Link the (PIC or non-PIC) object files of the given modules
> +    % into either an executable or a shared library (depending on the value
> +    % of the compile_to_shared_lib option). Name the executable or shared
> +    % library file after the value of the output_file_name option, if it is
> +    % specified, or if it not, the after the first module in ModulesToLink.
>      %
> -    % The Globals are supplied late to allow mercury_compile.m to
> -    % partially apply the preceding arguments.
> +    % This predicate is called mercury_compile_main.m during compiler

is called *by*

> +    % invocations which do NOT use "mmc --make". This means that it is used
> +    % only during compiler invocations where the target language is C.
>      %
> -:- pred link_module_list(io.text_output_stream::in,
> -    list(string)::in, list(string)::in, globals::in,
> +:- pred link_modules_into_executable_or_shared_library(
> +    io.text_output_stream::in, globals::in,
> +    list(module_name)::in, list(string)::in,
>      list(error_spec)::out, maybe_succeeded::out, io::di, io::uo) is det.
>
>  :- type linked_target_type
> @@ -107,11 +111,45 @@
>      ;       java_executable
>      ;       java_archive.
>
> -    % link(Globals, ProgressStream, TargetType, MainModuleName,
> -    %   ObjectFileNames, Specs, Succeeded, !IO)
> +    % link_files_into_executable_or_library(ProgressStream, Globals,
> +    %   LinkedTargetType, MainModuleName, FilesToLink, Specs, Succeeded, !IO):
>      %
> -:- pred link(globals::in, io.text_output_stream::in,
> -    linked_target_type::in, module_name::in, list(string)::in,
> +    % Link the given list of object files or object-like files into an
> +    % executable or library of some kind, which should be named after
> +    % MainModuleName.
> +    %
> +    % When targeting C, FilesToLink should be PIC or non-PIC object files.
> +    % When targeting Java, FilesToLink should be .jar files.

They should be .class files, not .jar files.

> +    % When targeting C#, FilesToLink should be CIL .dll files.
> +    %
> +    % Unlike the predicate above, we take as argument FilesToLink, not
> +    % ModulesToLink. This both allows and requires our callers (which are
> +    % in make.module_target.m and make.program_target.m) to handle
> +    % the inclusion in FilesToLink of every file to be linked that is NOT
> +    % the object file or object-like file of a Mercury module. This includes
> +    % the program's init file, and such auxiliary files as those used to
> +    % implement fact tables.
> +    %
> +    % Likewise, this predicate requires our caller to decide whether
> +    % we should create an executable or a library, and if the latter,
> +    % what kind of library. They pass that info to us in LinkedTargetType.
> +    %
> +    % Likewise, this predicate requires our caller to decide the name
> +    % of that executable or library. They pass that info in MainModuleName.
> +    %
> +    % XXX Having the names differ in the ending being "shared_library" vs
> +    % just "library" is not very effective in describing the differemce

/differemce/difference

> +    % between the this predicate and the one above. However, one cannot call
> +    % the above the non-mmc-make version and this one the mmc-make version,
> +    % because this one is used to help implement the one above.

As far as users of this module are concerned that is the distinction.

> +    % XXX If we moved the code for making the above decisions to this module
> +    % from make.program_target.m, we could maybe factor out commonalities with
> +    % the other predicate above. We would still need to export something like
> +    % this to make.module_target.m for creating .dll files.
> +    %
> +:- pred link_files_into_executable_or_library(io.text_output_stream::in,
> +    globals::in, linked_target_type::in, module_name::in, list(string)::in,
>      list(error_spec)::out, maybe_succeeded::out, io::di, io::uo) is det.

That looks fine otherwise.

Julien.