[clangd] Collect location of macro definition in the ParsedAST

[hokein]

Summary

allows semantic hightlighting macro definition

[hokein]

update

[hokein]

clang-tools-extra/clangd/ParsedAST.cpp

104 ↗

(On Diff #219067)

The name doesn't fit into what it does anymore, maybe rename to CollectMainFileMacroLocations?

[ilya-biryukov]

clang-tools-extra/clangd/ParsedAST.cpp

104 ↗

(On Diff #219067)

Agree, maybe even shorter - CollectMainFileMacros?

clang-tools-extra/clangd/ParsedAST.h

125 ↗

(On Diff #219067)

NIT: s/Does not include expansions/Does not include locations?

To avoid confusion, since they are definitions and expansions now.

126 ↗

(On Diff #219067)

Could we you rename this to MacroIdentifierLocs? Or something similar not mentioning expansions in the name.

MacroExpLocs could still be interpreted as locations of expansions, rather than locations of all macro identifiers.

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

234 ↗

(On Diff #219067)

We should probably keep the test as is and make sure we also collect inside the preamble.
It's ok to leave a FIXME and do this in a separate change, but let's not change the test to hide the fact that we are not highlighting macros inside the preamble anymore.

[hokein]

Rename all things.

[hokein]

Thanks for the suggestions on the new names.

[ilya-biryukov]

LGTM.

We should probably also take a look at highlighting macros inside the preamble part of the main file.
@hokein, are you planning to do this or should we just file a bug for this for now?

clang-tools-extra/clangd/SemanticHighlighting.cpp

38 ↗

(On Diff #219343)

NIT: use SourceLocation, it's just an int, so no need to use references here.

[hokein]

address comments

[hokein]

In D67264#1663285, @ilya-biryukov wrote:

LGTM.

We should probably also take a look at highlighting macros inside the preamble part of the main file.
@hokein, are you planning to do this or should we just file a bug for this for now?

I will file a bug in case we don't forget it.

clang-tools-extra/trunk/clangd/ParsedAST.h

public:
   const std::vector<Diag> &getDiagnostics() const;
 
   /// Returns the esitmated size of the AST and the accessory structures, in
   /// bytes. Does not include the size of the preamble.
   std::size_t getUsedBytes() const;
   const IncludeStructure &getIncludeStructure() const;
   const CanonicalIncludes &getCanonicalIncludes() const;
 
-  /// The start locations of all macro expansions spelled inside the main file.
-  /// Does not include expansions from inside other macro expansions.
-  llvm::ArrayRef<SourceLocation> getMainFileExpansions() const;
+  /// Gets all macro locations (definition, expansions) present in the main
+  /// file.
+  /// NOTE: macros inside the preamble are not included.
+  llvm::ArrayRef<SourceLocation> getMacros() const;
   /// Tokens recorded while parsing the main file.
   /// (!) does not have tokens from the preamble.
   const syntax::TokenBuffer &getTokens() const { return Tokens; }
 
 private:
   ParsedAST(std::shared_ptr<const PreambleData> Preamble,
             std::unique_ptr<CompilerInstance> Clang,
             std::unique_ptr<FrontendAction> Action, syntax::TokenBuffer Tokens,
private:
   std::unique_ptr<CompilerInstance> Clang;
   std::unique_ptr<FrontendAction> Action;
   /// Tokens recorded after the preamble finished.
   ///   - Includes all spelled tokens for the main file.
   ///   - Includes expanded tokens produced **after** preabmle.
   ///   - Does not have spelled or expanded tokens for files from preamble.
   syntax::TokenBuffer Tokens;
 
-  /// The start locations of all macro expansions spelled inside the main file.
-  /// Does not include expansions from inside other macro expansions.
-  std::vector<SourceLocation> MainFileMacroExpLocs;
+  /// The start locations of all macro definitions/expansions spelled **after**
+  /// preamble.
+  /// Does not include locations from inside other macro expansions.
+  std::vector<SourceLocation> MacroIdentifierLocs;
   // Data, stored after parsing.
   std::vector<Diag> Diags;
   // Top-level decls inside the current file. Not that this does not include
   // top-level decls from the preamble.
   std::vector<Decl *> LocalTopLevelDecls;
   IncludeStructure Includes;
   CanonicalIncludes CanonIncludes;
 };

clang-tools-extra/trunk/clangd/ParsedAST.cpp

protected:
   CreateASTConsumer(CompilerInstance &CI, llvm::StringRef InFile) override {
     return std::make_unique<DeclTrackingASTConsumer>(/*ref*/ TopLevelDecls);
   }
 
 private:
   std::vector<Decl *> TopLevelDecls;
 };
 
-// This collects macro expansions in the main file.
+// This collects macro expansions/definitions in the main file.
 // (Contrast with CollectMainFileMacros in Preamble.cpp, which collects macro
 // *definitions* in the preamble region of the main file).
-class CollectMainFileMacroExpansions : public PPCallbacks {
+class CollectMainFileMacros : public PPCallbacks {
   const SourceManager &SM;
   std::vector<SourceLocation> &MainFileMacroLocs;
 
+  void addLoc(SourceLocation Loc) {
+    if (!Loc.isMacroID() && isInsideMainFile(Loc, SM))
+      MainFileMacroLocs.push_back(Loc);
+  }
+
 public:
-  CollectMainFileMacroExpansions(const SourceManager &SM,
-                                 std::vector<SourceLocation> &MainFileMacroLocs)
+  CollectMainFileMacros(const SourceManager &SM,
       : SM(SM), MainFileMacroLocs(MainFileMacroLocs) {}
 
+  void MacroDefined(const Token &MacroNameTok,
+                    const MacroDirective *MD) override {
+    addLoc(MacroNameTok.getLocation());
+  }
   void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
                     SourceRange Range, const MacroArgs *Args) override {
-    SourceLocation L = MacroNameTok.getLocation();
-    if (!L.isMacroID() && isInsideMainFile(L, SM))
-      MainFileMacroLocs.push_back(L);
+    addLoc(MacroNameTok.getLocation());
   }
 };
 
 // When using a preamble, only preprocessor events outside its bounds are seen.
 // This is almost what we want: replaying transitive preprocessing wastes time.
 // However this confuses clang-tidy checks: they don't see any #includes!
 // So we replay the *non-transitive* #includes that appear in the main-file.
 // It would be nice to replay other events (macro definitions, ifdefs etc) but
ParsedAST::build(std::unique_ptr<clang::CompilerInvocation> CI,
   // Important: collectIncludeStructure is registered *after* ReplayPreamble!
   // Otherwise we would collect the replayed includes again...
   // (We can't *just* use the replayed includes, they don't have Resolved path).
   Clang->getPreprocessor().addPPCallbacks(
       collectIncludeStructureCallback(Clang->getSourceManager(), &Includes));
   // Collect the macro expansions in the main file.
   std::vector<SourceLocation> MainFileMacroExpLocs;
   Clang->getPreprocessor().addPPCallbacks(
-      std::make_unique<CollectMainFileMacroExpansions>(
-          Clang->getSourceManager(), MainFileMacroExpLocs));
+      std::make_unique<CollectMainFileMacros>(Clang->getSourceManager(),
+                                              MainFileMacroExpLocs));
 
   // Copy over the includes from the preamble, then combine with the
   // non-preamble includes below.
   CanonicalIncludes CanonIncludes;
   if (Preamble)
     CanonIncludes = Preamble->CanonIncludes;
   else
     CanonIncludes.addSystemHeadersMapping(Clang->getLangOpts());

 const Preprocessor &ParsedAST::getPreprocessor() const {
   return Clang->getPreprocessor();
 }
 
 llvm::ArrayRef<Decl *> ParsedAST::getLocalTopLevelDecls() {
   return LocalTopLevelDecls;
 }
 
-llvm::ArrayRef<SourceLocation> ParsedAST::getMainFileExpansions() const {
-  return MainFileMacroExpLocs;
+llvm::ArrayRef<SourceLocation> ParsedAST::getMacros() const {
+  return MacroIdentifierLocs;
 }
 
 const std::vector<Diag> &ParsedAST::getDiagnostics() const { return Diags; }
 
 std::size_t ParsedAST::getUsedBytes() const {
   auto &AST = getASTContext();
   // FIXME(ibiryukov): we do not account for the dynamically allocated part of
   // Message and Fixes inside each diagnostic.

 const CanonicalIncludes &ParsedAST::getCanonicalIncludes() const {
   return CanonIncludes;
 }
 
 ParsedAST::ParsedAST(std::shared_ptr<const PreambleData> Preamble,
                      std::unique_ptr<CompilerInstance> Clang,
                      std::unique_ptr<FrontendAction> Action,
                      syntax::TokenBuffer Tokens,
-                     std::vector<SourceLocation> MainFileMacroExpLocs,
+                     std::vector<SourceLocation> MacroIdentifierLocs,
                      std::vector<Decl *> LocalTopLevelDecls,
                      std::vector<Diag> Diags, IncludeStructure Includes,
                      CanonicalIncludes CanonIncludes)
     : Preamble(std::move(Preamble)), Clang(std::move(Clang)),
       Action(std::move(Action)), Tokens(std::move(Tokens)),
-      MainFileMacroExpLocs(std::move(MainFileMacroExpLocs)),
+      MacroIdentifierLocs(std::move(MacroIdentifierLocs)),
       Diags(std::move(Diags)),
       LocalTopLevelDecls(std::move(LocalTopLevelDecls)),
       Includes(std::move(Includes)), CanonIncludes(std::move(CanonIncludes)) {
   assert(this->Clang);
   assert(this->Action);
 }
 
 llvm::Optional<ParsedAST>

clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp

 public:
   HighlightingTokenCollector(ParsedAST &AST) : AST(AST) {}
 
   std::vector<HighlightingToken> collectTokens() {
     Tokens.clear();
     TraverseAST(AST.getASTContext());
     // Add highlightings for macro expansions as they are not traversed by the
     // visitor.
-    // FIXME: Should add highlighting to the macro definitions as well. But this
-    // information is not collected in ParsedAST right now.
-    for (const SourceLocation &L : AST.getMainFileExpansions())
-      addToken(L, HighlightingKind::Macro);
+    for (SourceLocation Loc : AST.getMacros())
+      addToken(Loc, HighlightingKind::Macro);
     // Initializer lists can give duplicates of tokens, therefore all tokens
     // must be deduplicated.
     llvm::sort(Tokens);
     auto Last = std::unique(Tokens.begin(), Tokens.end());
     Tokens.erase(Last, Tokens.end());
     // Macros can give tokens that have the same source range but conflicting
     // kinds. In this case all tokens sharing this source range should be
     // removed.

clang-tools-extra/trunk/clangd/unittests/ParsedASTTests.cpp

TEST(ParsedASTTest, CanBuildInvocationWithUnknownArgs) {
   Inputs.CompileCommand.CommandLine = {
       "clang", "-Xclang", "-fsome-unknown-flag", testPath("foo.cpp")};
   EXPECT_NE(buildCompilerInvocation(Inputs, IgnoreDiags), nullptr);
 }
 
 TEST(ParsedASTTest, CollectsMainFileMacroExpansions) {
   Annotations TestCase(R"cpp(
     #define MACRO_ARGS(X, Y) X Y
+    // - premable ends, macros inside preamble are not considered in main file.
     ^ID(int A);
     // Macro arguments included.
     ^MACRO_ARGS(^MACRO_ARGS(^MACRO_EXP(int), A), ^ID(= 2));
 
     // Macro names inside other macros not included.
-    #define FOO BAR
-    #define BAR 1
+    #define ^MACRO_ARGS2(X, Y) X Y
+    #define ^FOO BAR
+    #define ^BAR 1
     int A = ^FOO;
 
     // Macros from token concatenations not included.
-    #define CONCAT(X) X##A()
-    #define PREPEND(X) MACRO##X()
-    #define MACROA() 123
+    #define ^CONCAT(X) X##A()
+    #define ^PREPEND(X) MACRO##X()
+    #define ^MACROA() 123
     int B = ^CONCAT(MACRO);
     int D = ^PREPEND(A)
 
     // Macros included not from preamble not included.
     #include "foo.inc"
 
-    #define assert(COND) if (!(COND)) { printf("%s", #COND); exit(0); }
+    #define ^assert(COND) if (!(COND)) { printf("%s", #COND); exit(0); }
 
     void test() {
       // Includes macro expansions in arguments that are expressions
       ^assert(0 <= ^BAR);
     }
   )cpp");
   auto TU = TestTU::withCode(TestCase.code());
   TU.HeaderCode = R"cpp(
     #define ID(X) X
     #define MACRO_EXP(X) ID(X)
     MACRO_EXP(int B);
   )cpp";
   TU.AdditionalFiles["foo.inc"] = R"cpp(
     int C = ID(1);
     #define DEF 1
     int D = DEF;
   )cpp";
   ParsedAST AST = TU.build();
-  const std::vector<SourceLocation> &MacroExpansionLocations =
-      AST.getMainFileExpansions();
+  const std::vector<SourceLocation> &MacroExpansionLocations = AST.getMacros();
   std::vector<Position> MacroExpansionPositions;
   for (const auto &L : MacroExpansionLocations)
     MacroExpansionPositions.push_back(
         sourceLocToPosition(AST.getSourceManager(), L));
   EXPECT_EQ(MacroExpansionPositions, TestCase.points());
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp

)cpp",
         $LocalVariable[[GG]].$Method[[foo]](&$LocalVariable[[FF]]);
         $Class[[A]]<$Function[[foo]]> $LocalVariable[[AA]];
     )cpp",
       // Tokens that share a source range but have conflicting Kinds are not
       // highlighted.
       R"cpp(
       #define DEF_MULTIPLE(X) namespace X { class X { int X; }; }
       #define DEF_CLASS(T) class T {};
+      // Preamble ends.
       $Macro[[DEF_MULTIPLE]](XYZ);
       $Macro[[DEF_MULTIPLE]](XYZW);
       $Macro[[DEF_CLASS]]($Class[[A]])
-      #define MACRO_CONCAT(X, V, T) T foo##X = V
-      #define DEF_VAR(X, V) int X = V
-      #define DEF_VAR_T(T, X, V) T X = V
-      #define DEF_VAR_REV(V, X) DEF_VAR(X, V)
-      #define CPY(X) X
-      #define DEF_VAR_TYPE(X, Y) X Y
-      #define SOME_NAME variable
-      #define SOME_NAME_SET variable2 = 123
-      #define INC_VAR(X) X += 2
+      #define $Macro[[MACRO_CONCAT]](X, V, T) T foo##X = V
+      #define $Macro[[DEF_VAR]](X, V) int X = V
+      #define $Macro[[DEF_VAR_T]](T, X, V) T X = V
+      #define $Macro[[DEF_VAR_REV]](V, X) DEF_VAR(X, V)
+      #define $Macro[[CPY]](X) X
+      #define $Macro[[DEF_VAR_TYPE]](X, Y) X Y
+      #define $Macro[[SOME_NAME]] variable
+      #define $Macro[[SOME_NAME_SET]] variable2 = 123
+      #define $Macro[[INC_VAR]](X) X += 2
       $Primitive[[void]] $Function[[foo]]() {
         $Macro[[DEF_VAR]]($LocalVariable[[X]],  123);
         $Macro[[DEF_VAR_REV]](908, $LocalVariable[[XY]]);
         $Primitive[[int]] $Macro[[CPY]]( $LocalVariable[[XX]] );
         $Macro[[DEF_VAR_TYPE]]($Class[[A]], $LocalVariable[[AA]]);
         $Primitive[[double]] $Macro[[SOME_NAME]];
         $Primitive[[int]] $Macro[[SOME_NAME_SET]];
         $LocalVariable[[variable]] = 20.1;
         $Macro[[MACRO_CONCAT]](var, 2, $Primitive[[float]]);
         $Macro[[DEF_VAR_T]]($Class[[A]], $Macro[[CPY]](
               $Macro[[CPY]]($LocalVariable[[Nested]])),
             $Macro[[CPY]]($Class[[A]]()));
         $Macro[[INC_VAR]]($LocalVariable[[variable]]);
       }
       $Primitive[[void]] $Macro[[SOME_NAME]]();
       $Macro[[DEF_VAR]]($Variable[[XYZ]], 567);
       $Macro[[DEF_VAR_REV]](756, $Variable[[AB]]);
 
-      #define CALL_FN(F) F();
-      #define DEF_FN(F) void F ()
+      #define $Macro[[CALL_FN]](F) F();
+      #define $Macro[[DEF_FN]](F) void F ()
       $Macro[[DEF_FN]]($Function[[g]]) {
         $Macro[[CALL_FN]]($Function[[foo]]);
       }
     )cpp",
       R"cpp(
       #define fail(expr) expr
       #define assert(COND) if (!(COND)) { fail("assertion failed" #COND); }
+      // Preamble ends.
       $Primitive[[int]] $Variable[[x]];
       $Primitive[[int]] $Variable[[y]];
       $Primitive[[int]] $Function[[f]]();
       $Primitive[[void]] $Function[[foo]]() {
         $Macro[[assert]]($Variable[[x]] != $Variable[[y]]);
         $Macro[[assert]]($Variable[[x]] != $Function[[f]]());
       }
     )cpp",
EXPECT_THAT(ActualDiff,
               testing::UnorderedElementsAre(
                   testing::AllOf(LineNumber(3), EmptyHighlightings()),
                   testing::AllOf(LineNumber(4), EmptyHighlightings())));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang
 No newline at end of file