This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Avoids self references in transitive include test.
ClosedPublic

Authored by Mordante on Aug 27 2022, 3:59 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG9185d6e6bca8: [libc++] Avoids self references in transitive include test.
Summary

The output of --trace-includes starts with the header whose includes are
being processed. Since the sanitize script processed all lines this
include was added to the list of transitive includes. This looks odd
since it implies all headers have a cyclic dependency on themselves.
This change removes this self-include.

Instead of just dropping the first line extract that header and use it
to guard against cyclic dependencies in the header itself.

The regex used has a small improvement; don't capture groups that aren't
extracted.

Depends on D132284

Diff Detail

Event Timeline

Mordante created this revision.Aug 27 2022, 3:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2022, 4:00 AM
Mordante requested review of this revision.Aug 27 2022, 4:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2022, 4:00 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 456129.Aug 27 2022, 9:30 AM

Attempts to fix the CI.

ldionne accepted this revision.Aug 31 2022, 9:30 AM

Thanks, this LGTM and it is an oversight in the original implementation. I have a suggestion for simplifying this, but it's non-blocking.

libcxx/test/libcxx/transitive_includes.sanitize.py
39

Instead, perhaps it would be cleaner to figure out the top level header separately (pseudo-code, will need some changes):

get_top_level = lambda line: re.match(r'. .*(?:/|\\\\)include(?:/|\\\\)c\+\+(?:/|\\\\)v[0-9]+(?:/|\\\\)(.+)', line)
top_level_header = next(get_top_level_header(line) for line in lines if <matches>)

# Then do the rest exactly as it was
...

# Finally, simply remove the top-level header from the set of all headers
sorted(set(headers) - set(top_level_header))
This revision is now accepted and ready to land.Aug 31 2022, 9:30 AM
Mordante updated this revision to Diff 457293.Sep 1 2022, 8:50 AM

Improve the code a bit to make the loop simpler.
Give it a CI run before landing.

Mordante marked an inline comment as done.Sep 1 2022, 8:54 AM
Mordante added inline comments.
libcxx/test/libcxx/transitive_includes.sanitize.py
39

I agree the code can be a bit cleaner and I did a refactoring. I feel your solution looks a bit too clever; reading the finally it seems there is even an issue in this proposal.
sorted(set(headers) - set(top_level_header)) implies headers may contain a top_level_header. This should not happen, since this script does a cycle detection.

This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.