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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG with a couple of readability things



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:259
+    for (const FileEntry *FE = SM.getFileEntryForID(ID);
+         ID != SM.getMainFileID() && FE &&
+         !Includes.isSelfContained(*Includes.getID(FE));
----------------
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


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:331
+  TU.AdditionalFiles["indirection.h"] = R"cpp(
+    #include "unguarded.h"
+    )cpp";
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114623