This is an archive of the discontinued LLVM Phabricator instance.

[clangd] IncludeCleaner: Attribute symbols from non self-contained headers to their parents
ClosedPublic

Authored by kbobyrev on Nov 26 2021, 2:58 AM.

Details

Summary

When a symbol comes from the non self-contained header, we recursively uplift
the file we consider used to the first includer that has a header guard. We
need to do this while we still have FileIDs because every time a non
self-contained header is included, it gets a new FileID but is later
deduplicated by HeaderID and it's not possible to understand where it was
included from.

Based on D114370.

Diff Detail

Event Timeline

kbobyrev created this revision.Nov 26 2021, 2:58 AM
kbobyrev requested review of this revision.Nov 26 2021, 2:58 AM
sammccall accepted this revision.Nov 26 2021, 3:50 AM

LG with a couple of readability things

clang-tools-extra/clangd/IncludeCleaner.cpp
259

there's some subtle logic buried in here here:

  • the FE may be null, and if it is then we consider it the responsible header (rather than its include parent or possibly include child)
  • the HeaderID is never null

I'd prefer to see these expressed these more explicitly, and it's worth thinking if they're the right decisions or just provide the tersest loop :-)

Really I think pulling out a function: headerResponsible(FileID, ...) might make it possible to make this code clearer without cluttering the outer algorithm

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
331

This testcase is trying to cover too many bases, I think. It's complex and also doesn't manage to test everything.

The point of having "unguarded.h" reachable via two paths yielding different symbols seems to be that one of them can be used and the other unused, and we can only tell the difference by operating on FileIDs.
But because both variables are referenced here, we can't tell the difference and would get the same result by operating on HeaderIDs.

I think it'd be best to split it:

  • DistinctUnguardedInclusions which is but with indirection.h removed, only one style of header guard, and bar::Variable unreferenced
  • NonSelfContainedHeaders which just has main -> foo -> indirection -> unguarded, with no namespace tricks. Variable is referenced.

If you want to test the multiple styles of include guards that's OK, but probably belongs rather on the test for IncludeStructure::isSelfContained

This revision is now accepted and ready to land.Nov 26 2021, 3:50 AM
kbobyrev updated this revision to Diff 390051.Nov 26 2021, 7:15 AM
kbobyrev marked 2 inline comments as done.

Address review comments.

kbobyrev updated this revision to Diff 390052.Nov 26 2021, 7:19 AM

Add some docs for the helper function.

This revision was landed with ongoing or failed builds.Nov 26 2021, 7:21 AM
This revision was automatically updated to reflect the committed changes.