[libcxx-commits] [PATCH] D133127: [libc++] Adds include graph test.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: libcxx/test/libcxx/trace_includes.sanitize.py:15-16
+
+# Note --trace-includes keeps cycles in the output. However when not in a cycle
+# duplicates are not in the tree. For example the header version which is
+# included by most headers only appears once. This output can't be used to see
----------------
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133127/new/

https://reviews.llvm.org/D133127