[m-rev.] for post-commit review: touched_files

[Zoltan Somogyi]

On 2023-12-08 15:06 +11:00 AEDT, "Peter Wang" <novalazy at gmail.com> wrote:
> On Fri, 08 Dec 2023 12:54:41 +1100 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
>> 
>> On 2023-12-08 12:42 +11:00 AEDT, "Peter Wang" <novalazy at gmail.com> wrote:
>> >> +    TouchedFiles = touched_files(TouchedTargetFiles, TouchedTimestampFiles),
>> >>      list.map_foldl2(
>> >>          get_target_timestamp(ProgressStream, Globals, do_not_search),
>> >>          TouchedTargetFiles, TargetTimestamps, !Info, !IO),
>> > 
>> > The name "TouchedTimestampFiles" (here and elsewhere),
>> > when the list may include non-timestamp files, could be confusing.
>> 
>> Do you have a more descriptive name that is not too long? I tried to think of one,
>> but failed. Basically, *most* of those files *are* timestamp files, but *all* of them,
>> even the ones that are e.g. .c or .o files, are files that are passed along so that
>> code can call "stat" on them and see when they were last modified. In *that* sense,
>> though not in the usual one, they are timestamp files, and the comment on
>> the second field of the new type should explain that fact.
> 
> How about TouchedTargetFiles and TouchedNonTargetFiles?
> 
> Or, since you've grouped together the arguments, there's not as much
> reason to keep timestamp files and foreign code files in the same list.

I like the second suggestion better, and will implement it.

>> I was actually more concerned about the other part of the name, "Touched".
>> The code that computes the touched_files structure does not actually touch
>> any of those files; it just computes the set of files that building the chosen
>> target *would* touch. Again, I tried and failed to find a name that expresses
>> that fact while not being too long, so I kept the old naming scheme while
>> explaining the semantics in the new type's comment.
> 
> They are potentially touched by the compiler, so that I think the name
> is okay. {potentially,maybe}_touched_files is a bit long.

My thought exactly, but see below.

> That prompted me to look at the touched_files comment again.
> What did you mean by "foreign_include_module declarations"?

I was mixing them up with files named in foreign_{code,decl} and include_file.
I will fix it.

> I skimmed it too quickly and thought you meant foreign_decl or
> foreign_code decls using the 'include_file' feature.
> Reading the code again, files referenced by 'include_file'
> are NOT included in the tf_timestamp_files lists,

Agreed; the only foreign code files included are the ones mmc generates
for fact tables. Unfortunately, the predicate that decides this has a misleading
name :-(. I will fix it.

> which is good since the compiler only ever reads from those files,
> never touching them.

Which is why I thought it made sense to include them in the list of files
which, if recently updated, should trigger the rebuilding of the module's
compiler-generated target files. However, I see that this dependency is
handled by make.dependencies.m instead.

In writing this reply, I dug deeper into how the filenames in the
second arg of touched_files are used. I found code that did not document
what it did and why. I think I have figured that out, and will modify the code
to make that plain. I intend to post it for your review later today. I intend to
rename touched_files to make_lhs_files, because I now think that better
describes how the info in it is used. Would that work for you?

Zoltan.