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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
