[m-rev.] for review: Reset target file timestamp cache before installing library grade.

[Zoltan Somogyi]

On 2023-12-12 14:39 +11:00 AEDT, "Peter Wang" <novalazy at gmail.com> wrote:
> diff --git a/compiler/make.program_target.m b/compiler/make.program_target.m
> index 385a07f2f..3a3144297 100644
> --- a/compiler/make.program_target.m
> +++ b/compiler/make.program_target.m
> @@ -1847,6 +1847,11 @@ install_library_grade(LinkSucceeded0, ModuleName, AllModules,
>          make_info_set_dep_file_status_map(StatusMap, !Info),
>          make_info_set_option_args(OptionArgs, !Info),
>  
> +        % Reset the target file timestamp cache, as the information it contains
> +        % is not valid for the changed grade and grade-subdir setting.
> +        make_info_set_target_file_timestamps(init_target_file_timestamps,
> +            !Info),
> +
>          % Building the library in the new grade is done in a separate process
>          % to make it easier to stop and clean up on an interrupt.
>          globals.lookup_bool_option(LibGlobals, very_verbose, VeryVerbose),

This code to clear out the cache is done before installing grade.
If you are installing several grades, this will clean out the cache before each grade.
However, is it possible for a compiler invocation to (a) install one or more grades,
and then (b) do something else? If yes, then we will need to clear out the cache
before step b as well.

> +:- pred verify_cached_target_file_timestamp(io.text_output_stream::in,
> +    globals::in, maybe_search::in, target_file::in, timestamp::in,
> +    make_info::in, make_info::out, io::di, io::uo) is det.
> +
> +verify_cached_target_file_timestamp(ProgressStream, Globals, Search,
> +        TargetFile, Timestamp0, !Info, !IO) :-
> +    ForSearch = maybe_search_to_maybe_for_search(Search),
> +    module_maybe_nested_target_file_to_file_name(ProgressStream,
> +        Globals, $pred, ForSearch, TargetFile, FileName, !Info, !IO),
> +    get_target_timestamp_2(ProgressStream, Globals,
> +        Search, TargetFile, FileName, MaybeTimestamp, !Info, !IO),

Until now, get_target_timestamp_2 was called only from get_target_timestamp,
and its analysis registry version, so it was ok that its name was not descriptive.
With the addition of this call, it needs a descriptive name. Sadly, I don't know
enough the motivation for what exactly it does to propose such a name.

> +    (
> +        MaybeTimestamp = ok(Timestamp),
> +        ( if Timestamp0 \= Timestamp then

I would reverse this test, and exchange the then- and else-parts.
I would also rename Timestamp0 and Timestamp; they are not initial
and final versions, but come from different sources. I would put the source
in each name.

> +            string.format(
> +                "target file timestamp differs: %s (cached) vs %s (actual)",
> +                [s(timestamp_to_string(Timestamp0)),
> +                s(timestamp_to_string(Timestamp))], Msg),
> +            unexpected($pred, Msg)
> +        else
> +            true
> +        )
> +    ;
> +        MaybeTimestamp = error(Error),
> +        string.format(
> +            "target file timestamp differs: %s (cached) vs %s (actual)",
> +            [s(timestamp_to_string(Timestamp0)), s(Error)], Msg),
> +        unexpected($pred, Msg)
> +    ).
> +

Apart from that, the diff is fine.

Zoltan.