This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] adds an include-what-you-use (IWYU) mapping file
ClosedPublic

Authored by cjdb on Nov 17 2022, 12:06 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGab4664808281: [libcxx] adds an include-what-you-use (IWYU) mapping file
Summary

This makes it possible for programmers to run IWYU and get more accurate
standard library inclusions. Prior to this commit, the following program
would be transformed thusly:

cpp
// Before
 #include <algorithm>
 #include <vector>

void f() {
  auto v = std::vector{0, 1};
  std::find(std::ranges::begin(v), std::ranges::end(v), 0);
}
cpp
// After
 #include <__algorithm/find.h>
 #include <__ranges/access.h>
 #include <vector>
...

There are two ways to fix this issue: to use comment pragmas
on every private include, or to write a canonical mapping file
that provides the tool with a manual on how libc++ is laid out. Due to
the complexity of libc++, this commit opts for the latter, to maximise
correctness and minimise developer burden.

To mimimise developer updates to the file, it makes use of wildcards
that match everything within listed subdirectories. A script has also
been added to ensure that the mapping is always fresh in CI, and makes
the process a single step.

Finally, documentation has been added to inform users that IWYU is
supported, and what they need to do in order to leverage the mapping
file.

Closes #56937.

Diff Detail

Event Timeline

cjdb created this revision.Nov 17 2022, 12:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2022, 12:06 AM
Herald added a subscriber: arichardson. · View Herald Transcript
cjdb requested review of this revision.Nov 17 2022, 12:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2022, 12:06 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Thanks! I think I really like this. We could also potentially use this as a tool to aid our removal of transitive includes -- arguably, if we provide a tool to automatically fix people's code, it may become more reasonable to break people's code through transitive include removals a bit more eagerly. I'm not suggesting we do that for now, just mentioning it.

libcxx/include/libcxx.imp
1

This patch doesn't ship this file with the libc++ headers. Did you mean to do that?

libcxx/utils/generate_iwyu_mapping.py
1

Just to clarify, we will not need to change this script unless we add a new detail header at the top level (which should be rare), right?

cjdb updated this revision to Diff 476294.Nov 17 2022, 5:17 PM
  • adds libcxx.imp to include/CMakeLists.txt
  • implements panic()
  • regenerates libcxx.imp so that it has a newline before EOF
cjdb marked 2 inline comments as done.Nov 17 2022, 5:27 PM

I'm not suggesting we do that for now, just mentioning it.

Ack. Some thoughts below for when you do entertain this more seriously.

Thanks! I think I really like this. We could also potentially use this as a tool to aid our removal of transitive includes -- arguably, if we provide a tool to automatically fix people's code, it may become more reasonable to break people's code through transitive include removals a bit more eagerly.

Before committing to this, it'd be good to survey the trust folks have in IWYU (my hunch is that it's very low); and to keep the transitive headers for multiple years behind a macro, for folks unable to use IWYU for whatever reason (e.g. perhaps a library doesn't have a mapping file and has lots of detail headers that make using IWYU unreasonable).

libcxx/include/libcxx.imp
1

Oops, nice catch!

libcxx/utils/generate_iwyu_mapping.py
1

Yeah, that and if a specific directory needs mapping (e.g. say you add __detail/).

cjdb updated this revision to Diff 476298.Nov 17 2022, 5:38 PM
cjdb marked 2 inline comments as done.
  • cleans up documentation issue
cjdb updated this revision to Diff 476318.Nov 17 2022, 7:18 PM
  • edits generate_header_tests.py so that it excludes libcxx.imp, which isn't a header
cjdb updated this revision to Diff 476322.Nov 17 2022, 8:05 PM
  • teaches lint_headers.sh.py about libcxx.imp
ldionne accepted this revision.Nov 18 2022, 2:17 PM

Thanks!

LGTM, but can you please undo the formatting-only changes in libcxx/utils/generate_header_tests.py? I don't want to block the review on this so I'm approving right away, but I'd like those to be undone and pursued in a separate patch (if at all -- IMO the previous formatting was fine).

This revision is now accepted and ready to land.Nov 18 2022, 2:17 PM
jloser added a subscriber: jloser.Nov 18 2022, 5:08 PM

Thanks so much for working on this, @cjdb! I'm excited to try it out.

cjdb added a comment.Nov 20 2022, 4:53 PM

Thanks!

LGTM, but can you please undo the formatting-only changes in libcxx/utils/generate_header_tests.py? I don't want to block the review on this so I'm approving right away, but I'd like those to be undone and pursued in a separate patch (if at all -- IMO the previous formatting was fine).

Yeah, sorry about that: my browser autoformats on save and since there's no .pyformat yapf just does whatever. I'll fix it up!

Before submitting: is the clang-cl static CI job a concern?

Thanks!

LGTM, but can you please undo the formatting-only changes in libcxx/utils/generate_header_tests.py? I don't want to block the review on this so I'm approving right away, but I'd like those to be undone and pursued in a separate patch (if at all -- IMO the previous formatting was fine).

Yeah, sorry about that: my browser autoformats on save and since there's no .pyformat yapf just does whatever. I'll fix it up!

Before submitting: is the clang-cl static CI job a concern?

Looks unrelated, I think you can land with @ldionne's comments addressed.

This revision was landed with ongoing or failed builds.Nov 21 2022, 5:20 PM
This revision was automatically updated to reflect the committed changes.
ldionne added inline comments.Nov 23 2022, 5:47 AM
libcxx/utils/generate_iwyu_mapping.py
39

It seems that this is only supported starting with Python 3.10.

I'm not sure what's the oldest version that LLVM supports, but I think it's older than 3.10, so I'll change this to an if-else chain. No action item, I just wanted to give a heads up.

cjdb added inline comments.Nov 28 2022, 9:24 AM
libcxx/utils/generate_iwyu_mapping.py
39

Thanks, I'll keep this in mind for next time. I was a bit apprehensive about using the feature because it was so new, but it looked like CI was happy with it, so I thought it would be okay.