[PATCH] D111698: [clangd] IncludeCleaner: Handle macros coming from ScratchBuffer

[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 the new test



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:124
     const auto &Exp = SM.getSLocEntry(FID).getExpansion();
-    add(Exp.getSpellingLoc());
+    // For nested macro expansions, the spelling location can be within a
+    // temporary buffer that Clang creates (scratch space or ScratchBuffer).
----------------
This is about token pasting, nested macro expansions are neither necessary nor sufficient.


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:198
+  auto HeaderID = Includes.getID(*Entry);
+  EXPECT_THAT(ReferencedFiles, UnorderedElementsAre(HeaderID));
+  EXPECT_THAT(getUnused(Includes, ReferencedFiles), ::testing::IsEmpty());
----------------
Nit: I think this should be asserting on FileIDs (i.e. before translation). It's narrower, and the contract is that findReferecedFiles should filter them out already.


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:198
+  auto HeaderID = Includes.getID(*Entry);
+  EXPECT_THAT(ReferencedFiles, UnorderedElementsAre(HeaderID));
+  EXPECT_THAT(getUnused(Includes, ReferencedFiles), ::testing::IsEmpty());
----------------
sammccall wrote:
> Nit: I think this should be asserting on FileIDs (i.e. before translation). It's narrower, and the contract is that findReferecedFiles should filter them out already.
Comment: no "<scratch space>" FID


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:199
+  EXPECT_THAT(ReferencedFiles, UnorderedElementsAre(HeaderID));
+  EXPECT_THAT(getUnused(Includes, ReferencedFiles), ::testing::IsEmpty());
+}
----------------
Comment: should not crash due to "files" missing from include structure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111698