[m-rev.] for post-commit review: separate two tasks

[Peter Wang]

On Sun, 10 Dec 2023 15:57:46 +1100 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> Move part of a predicate to its only caller.
> 
> compiler/make.dependencies.m:
>     Use a bespoke type to represent the result of the search for prerequisite
>     files, to help clarify new code in the change below.
> 
> compiler/make.module_target.m:
>     The make_dependency_file predicate used to have three lines to make
>     the rhs files of an action (which it called the dependency files),
>     and dozens of lines to help decide whether to execute the action.
>     Move those three lines to its parent, and rename the predicate
>     to reflect the one job of the remaining code.
> 
>     Simplify the code around the old call to make_dependency_file
>     by not trying to decide whether we *should* execute the make action
>     when we have already determined that we *cannot* execute it, due to
>     the making of some rhs file having failed.
> 
>     Document a recursive call.
> 
>     Delete a recently-obsoleted XXX.

> diff --git a/compiler/make.module_target.m b/compiler/make.module_target.m
> index 0202d187f..6d1d9095d 100644
> --- a/compiler/make.module_target.m
> +++ b/compiler/make.module_target.m
...
> @@ -220,27 +221,46 @@ make_module_target_file_main_path(ExtraOptions, ProgressStream, Globals,
>      maybe_write_msg(ProgressStream, CheckingMsg, !IO),
>  
>      find_direct_prereqs_of_target_file(ProgressStream, Globals,
> -        CompilationTaskType, ModuleDepInfo, TargetFile,
> -        DepsSucceeded, DepFilesSet, !Info, !IO),
> -    DepFilesToMake = set.to_sorted_list(DepFilesSet),
> +        CompilationTaskType, ModuleDepInfo, TargetFile, RhsResult,
> +        !Info, !IO),
>  
>      KeepGoing = make_info_get_keep_going(!.Info),
>      ( if
> -        DepsSucceeded = did_not_succeed,
> +        RhsResult = could_not_find_some_prereqs(_),
>          KeepGoing = do_not_keep_going
>      then
>          LhsResult = rhs_error
>      else
> -        make_dependency_files(ProgressStream, Globals,
> -            TargetFile, TargetFileName, DepFilesToMake,
> -            MakeLhsFiles, LhsResult0, !Info, !IO),
> +        ( RhsResult = could_not_find_some_prereqs(RhsDepFileSet)
> +        ; RhsResult = found_all_prereqs(RhsDepFileSet)
> +        ),
> +        % XXX MAKE sort
> +        RhsDepFiles = set.to_sorted_list(RhsDepFileSet),
> +        % Build the files on the rhs.
> +        foldl2_make_module_targets(KeepGoing, [], ProgressStream, Globals,
> +            RhsDepFiles, MakeRhsFilesSucceeded, !Info, !IO),
>          (
> -            DepsSucceeded = succeeded,
> -            LhsResult = LhsResult0
> +            MakeRhsFilesSucceeded = did_not_succeed,
> +            debug_make_msg(Globals,
> +                string.format("%s: error making dependencies\n",
> +                    [s(TargetFileName)]),
> +                RhsErrorDebugMsg),
> +            maybe_write_msg(ProgressStream, RhsErrorDebugMsg, !IO),
> +            LhsResult = rhs_error
> +        ;
> +            MakeRhsFilesSucceeded = succeeded,
> +            % We succeeded in making RhsDepFiles. Were these all the
> +            % prerequisities? If not, then we cannot build the rhs files.

I think this should say:

    We succeeded in making RhsDepFiles, but if there are prerequisities
    that we could not find, then we cannot build the lhs files.

> +            (
> +                RhsResult = found_all_prereqs(_),
> +                must_or_should_we_rebuild_lhs(ProgressStream, Globals,
> +                    TargetFile, TargetFileName, RhsDepFiles,
> +                    MakeLhsFiles, LhsResult, !Info, !IO)
>              ;
> -            DepsSucceeded = did_not_succeed,
> +                RhsResult = could_not_find_some_prereqs(_),
>                  LhsResult = rhs_error
>              )
> +        )
>      ),
>      (
>          LhsResult = rhs_error,

> +must_or_should_we_rebuild_lhs(ProgressStream, Globals,
> +        TargetFile, TargetFileName, RhsFiles, MakeLhsFiles, LhsResult,
> +        !Info, !IO) :-
>      % Check whether all the lhs files exist, because if some are missing,
>      % then we need to execute the action.
>      MakeLhsFiles = make_lhs_files(DatelessLhsTargetFiles,
> @@ -347,13 +349,14 @@ make_dependency_files(ProgressStream, Globals, TargetFile, TargetFileName,
>                  !Info, !IO),
>              AllLhsTimestamps = DatelessLhsFileTimestamps ++
>                  LhsDateFileTimestamps ++ LhsForeignCodeFileTimestamps,
> -                find_oldest_lhs_file(AllLhsTimestamps,
> -                    MaybeOldestLhsTimestamp),
> +            find_oldest_lhs_file(AllLhsTimestamps, MaybeOldestLhsTimestamp),
> +            % This predicate gets called only if the (re)building
> +            % of the rhs only if succeeded.

of the rhs files succeeded.

> +            MakeRhsFilesSucceeded = succeeded,
>              should_we_rebuild_lhs(ProgressStream, Globals, TargetFileName,
> -                    MaybeOldestLhsTimestamp, MakeDepsSucceeded, DepFilesToMake,
> +                MaybeOldestLhsTimestamp, MakeRhsFilesSucceeded, RhsFiles,
>                  LhsResult, !Info, !IO)
>          )
> -        )
>      ).

The rest is fine.

Peter