This is an archive of the discontinued LLVM Phabricator instance.

[libc++][test] Improves transitive includes.
ClosedPublic

Authored by Mordante on Sep 1 2022, 10:23 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG34dfdd17af19: [libc++][test] Improves transitive includes.
Summary

This test generates the include graph of the Standard headers of libc++ in
a CSV file. This was originally used to generate graphviz dot files. During
review it was noticed these files have all information needed to replace
the current transitive includes. Therefore the output, with the same information as the .dot file is stored in a .csv file. This removes
all the existing transitive include files.

The .cvs can be converted by a .dot file by the script in D134188.

Diff Detail

Event Timeline

Mordante created this revision.Sep 1 2022, 10:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2022, 10:23 AM
Mordante requested review of this revision.Sep 1 2022, 10:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2022, 10:23 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 457610.Sep 2 2022, 8:48 AM

Attempts to fix some CI issues.

ldionne requested changes to this revision.Sep 6 2022, 11:02 AM
ldionne added a subscriber: ldionne.

I have a few questions:

  1. Does this replace the existing graph_header_deps.py script? I strongly think we should have only one script that does this. I think it's fine if the new script doesn't support all the features that the old script did at the benefit of being correct w.r.t. conditional includes.
  2. Is there a reason why we're not using the output already generated in the libcxx/test/libcxx/transitive_includes/<std> directories?

Summary from our live review discussion:

I think we should start storing the graph of headers in the expected.foo files, but not their transitive closure. Then, we can reuse that as-is from graph_header_dependencies.py, and generate their transitive closure from the transitive_includes.sh.cpp test.

libcxx/test/libcxx/trace_includes.sanitize.py
15–16 ↗(On Diff #457610)

However when not in a cycle duplicates are not in the tree.

I have trouble understanding this sentence. Do you mean:

There are no duplicates in the tree unless a header is part of a cycle.

This revision now requires changes to proceed.Sep 6 2022, 11:02 AM
Mordante updated this revision to Diff 460984.Sep 17 2022, 3:25 AM

Repurposed this patch based on review comments and a private discussion with @ldionne.

Mordante updated this revision to Diff 460988.Sep 17 2022, 4:18 AM

Fixes CI.

Mordante updated this revision to Diff 461147.Sep 18 2022, 11:43 PM

Fixes Python 3.8 compatibility.

Mordante retitled this revision from [libc++] Adds include graph test. to [libc++][test] Improves transitive includes..Sep 19 2022, 8:51 AM
Mordante edited the summary of this revision. (Show Details)
ldionne accepted this revision.Sep 20 2022, 9:43 AM

LGTM w/ my suggestions. Thanks a lot for working on this simplification!

libcxx/test/libcxx/transitive_includes_to_csv.py
24

Python has a csv module you could use instead.

93–94

Can we get a super simple argparse here? It would be helpful if someone wants to call the script with --help.

libcxx/utils/ci/run-buildbot
191 ↗(On Diff #461147)

We could use a space instead of a tab as a delimiter.

This revision is now accepted and ready to land.Sep 20 2022, 9:43 AM
ldionne added inline comments.Sep 20 2022, 9:46 AM
libcxx/test/libcxx/transitive_includes.sanitize.py
50

I don't think this is used.

Mordante marked 4 inline comments as done.Sep 20 2022, 10:50 PM
Mordante added inline comments.
libcxx/test/libcxx/transitive_includes_to_csv.py
24

I had a look at it and it doesn't seem to be very useful in this case. Here we only write CSV to the output.

Mordante updated this revision to Diff 461796.Sep 20 2022, 10:56 PM
Mordante marked an inline comment as done.

Rebased and addresses review comments.

Mordante updated this revision to Diff 462706.Sep 25 2022, 2:41 AM

Rebased to test CI.

This revision was automatically updated to reflect the committed changes.
libcxx/test/libcxx/transitive_includes/cxx2b/expected.valarray