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

Peter Wang novalazy at gmail.com
Fri Dec 8 15:06:42 AEDT 2023


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 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.

That prompted me to look at the touched_files comment again.
What did you mean by "foreign_include_module declarations"?
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,
which is good since the compiler only ever reads from those files,
never touching them.

Peter


More information about the reviews mailing list