[m-rev.] for review: d_file_deps improvements

[Peter Wang]

On Sat, 09 Aug 2025 17:42:11 +0200 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> For review by anyone.
> 
> Zoltan.

> Partially clarify and partially fix d_file_deps.
...
> 
> compiler/d_file_deps.m:
>     Stop requiring the caller of construct_d_file_deps_gendep to pass
>     an aug_compilation_unit; get it to pass instead two of its components,
>     the only ones that are meaningful and also the only ones we actually need:
>     the parse_tree of the module in question and its baggage.
> 
>     Eliminate some unnecessary differences between compute_d_file_deps_gendep
>     and compute_d_file_deps_hlds by replacing some existing tests for just
>     --intermod-opt with tests for *either* --intermod-op *or* --use-opt-files,

--intermod-opt

> diff --git a/compiler/d_file_deps.m b/compiler/d_file_deps.m
> index 7b0fe71f3..1e44a73a4 100644
> --- a/compiler/d_file_deps.m
> +++ b/compiler/d_file_deps.m

> @@ -273,18 +273,23 @@ lookup_burdened_module_in_deps_map(DepsMap, ModuleName) = ModuleDepInfo :-
>  
>  %---------------------------------------------------------------------------%
>  
> -construct_d_file_deps_gendep(Globals, DepGraphs, BurdenedAugCompUnit,
> +construct_d_file_deps_gendep(Globals, DepGraphs, Baggage, ParseTreeModuleSrc,
>          DFileDeps) :-
> -    BurdenedAugCompUnit = burdened_aug_comp_unit(_Baggage, AugCompUnit),
> -    % XXX The reason why we ignore all the fields of AugCompUnit except
> -    % ParseTreeModuleSrc is that our caller gives us a BurdenedAugCompUnit
> -    % in which the AugCompUnit part contains nothing *but* the
> -    % ParseTreeModuleSrc. The info in DepGraphs is supposed to replace
> -    % this info, but it does not, and maybe cannot do so completely.
> -    AugCompUnit = aug_compilation_unit(ParseTreeModuleSrc,
> -        _, _, _, _, _, _, _, _),
> -    DepGraphs = dep_graphs(IntDepsGraph, ImpDepsGraph, IndirectDepsGraph,
> -        IndirectOptDepsGraph, TransOptDepsGraph, FullTransOptOrder),
> +    % While construct_d_file_deps_hlds takes an AugCompUnit as input,
> +    % we only get a ParseTreeModuleSrc. Some of the fields of an AugCompUnit,
> +    % such as acu_ancestor_ints, are easily simulated here, while others,
> +    % such as acu_indirect_int2s, would require substantial code to replicate,
> +    % and at the moment, this predicate does not even attempt to do so.
> +    % XXX The extent to which DepGraphs can replace the info need from
> +    % AugCompUnit is questionable.

needed

> @@ -1698,8 +1772,6 @@ generate_dep_file_exec_library_targets(Globals, ModuleName,
>          "$(patsubst %.o,%.$(EXT_FOR_PIC_OBJECTS)," ++
>          "$(foreach @," ++ ModuleMakeVarName ++ ",$(ALL_MLOBJS)))",
>  
> -    NL_All_MLObjs = "\\\n\t\t" ++ All_MLObjs,
> -
>      % When compiling to C, we want to include $(foo.cs) first in
>      % the dependency list, before $(foo.os).
>      % This is not strictly necessary, since the .$O files themselves depend
> @@ -1712,26 +1784,30 @@ generate_dep_file_exec_library_targets(Globals, ModuleName,
>      ModuleMakeVarNameClasses = "$(" ++ ModuleMakeVarName ++ ".classes)",
>  
>      ModuleMakeVarNameOs = "$(" ++ ModuleMakeVarName ++ ".all_os)",
> -    NonJavaMainRuleAction1Line1 =
> -        "$(ML) $(ALL_GRADEFLAGS) $(ALL_MLFLAGS) -- $(ALL_LDFLAGS) " ++
> -            "$(EXEFILE_OPT)" ++ ExeFileName ++ "$(EXT_FOR_EXE) " ++
> -            InitObjFileName ++ " \\",
> -    NonJavaMainRuleAction1Line2 =
> -        "\t" ++ ModuleMakeVarNameOs ++ " " ++ NL_All_MLObjs ++
> -            " $(ALL_MLLIBS)",
> +    CMainRuleAction1Line1 =
> +        "$(ML) $(ALL_GRADEFLAGS) $(ALL_MLFLAGS) -- $(ALL_LDFLAGS)",
> +    CMainRuleAction1Line2 =
> +        "$(EXEFILE_OPT)" ++ ExeFileName ++ "$(EXT_FOR_EXE)",
> +    CMainRuleAction1Line3 =
> +        InitObjFileName ++ " " ++ ModuleMakeVarNameOs,
> +    CMainRuleAction1Line4 =
> +        All_MLObjs ++ " $(ALL_MLLIBS)",
> +    CMainRuleAction1Lines = make_multiline_action(
> +        [CMainRuleAction1Line1, CMainRuleAction1Line2,
> +        CMainRuleAction1Line3, CMainRuleAction1Line4]),

Naming each line with a variable seems to make it less readable,
but anyway.

> diff --git a/library/digraph.m b/library/digraph.m
> index 5c239d8ec..a084be78e 100644
> --- a/library/digraph.m
> +++ b/library/digraph.m
...
>  
> -    % tc(G, TC) is true if TC is the transitive closure of G.
> +    % Synonyms for tc/1.
>      %
>  :- func tc(digraph(T)) = digraph(T).
>  :- pred tc(digraph(T)::in, digraph(T)::out) is det.

Synonyms for transitive_closure/1.

> +    % Synonyms for rtc/1.
> +    %
>  :- func rtc(digraph(T)) = digraph(T).
>  :- pred rtc(digraph(T)::in, digraph(T)::out) is det.

Synonyms for reflexive_transitive_closure/1

I paged through the diff but I can't say I reviewed it.
I'm fine with you committing it.

Peter