[clangd] IncludeCleaner: Handle macros coming from ScratchBuffer

[kbobyrev]

Mention the ScratchBuffer explicitly in the test.

[kbobyrev]

Rename the test to have more meaning.

[sammccall]

clang-tools-extra/clangd/IncludeCleaner.cpp

124–125

Add a comment

126

I'm not 100% sure these should be unconditional. It's *probably* right, but...
Can you add a test of the form:

#define ab x
#define concat(x, y) x##y
int foo(a, b);

and verify that we get the expected set of file IDs when seeding with the location of the VarDecl x?

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

170

this doesn't seem to test very much, a comment should indicate that this is guarding against a crash.

Ideally we'd (also?) have a test that calls findReferencedFiles and actually assert which IDs we get, rather than run some big blob of code and hope not to crash.

[kbobyrev]

Address review comments.

[kbobyrev]

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

170

Good point, I just ran into this for D111711:

foo.h

void foo();

foo.cpp

void foo() {}

computeUnusedIncludes() does not return foo.h but it wasn't processed (e.g. its ID was never in the ReferencedFiles in the first place) :(

[sammccall]

LG with the new test

clang-tools-extra/clangd/IncludeCleaner.cpp

124–125

This is about token pasting, nested macro expansions are neither necessary nor sufficient.

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

198

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.

198

Comment: no "<scratch space>" FID

199

Comment: should not crash due to "files" missing from include structure

[kbobyrev]

Stash

[kbobyrev]

Resolve review comments.

[kbobyrev]

Fix comment.

clang-tools-extra/clangd/IncludeCleaner.cpp

void add(FileID FID, SourceLocation Loc) {
     if (Loc.isFileID()) {
       Files.insert(FID);
       return;
     }
     // Don't process the same macro FID twice.
     if (!Macros.insert(FID).second)
       return;
     const auto &Exp = SM.getSLocEntry(FID).getExpansion();
+    if (!SM.isWrittenInScratchSpace(Exp.getSpellingLoc()))
       add(Exp.getSpellingLoc());
     add(Exp.getExpansionLocStart());
     add(Exp.getExpansionLocEnd());
   }
 };
 
 } // namespace
 
 ReferencedLocations findReferencedLocations(ParsedAST &AST) {
   ReferencedLocations Result;

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

TEST(IncludeCleaner, GetUnusedHeaders) {
   std::vector<std::string> UnusedHeaders;
   UnusedHeaders.reserve(UnusedIncludes.size());
   for (const auto &Include : UnusedIncludes)
     UnusedHeaders.push_back(Include->Written);
   EXPECT_THAT(UnusedHeaders,
               UnorderedElementsAre("\"unused.h\"", "\"dir/unused.h\""));
 }
 
+TEST(IncludeCleaner, MacroPastingOperator) {
+  TestTU TU;
+  TU.Filename = "foo.cpp";
+  TU.Code = R"cpp(
+    #include "macro_pasting_operator.h"
+
+    using flags::FLAGS_FOO;
+    )cpp";
+  // The pasting operator in combination with DEFINE_FLAG will create a buffer
+  // with `flags::FLAGS_FOO` that will have FileID but not FileEntry.
+  TU.AdditionalFiles["macro_pasting_operator.h"] = R"cpp(
+    #define DEFINE_FLAG(X) \
+    namespace flags { \
+    int FLAGS_##X; \
+    } \
+
+    DEFINE_FLAG(FOO)
+    )cpp";
+  ParsedAST AST = TU.build();
+  EXPECT_THAT(computeUnusedIncludes(AST), testing::IsEmpty());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang