[PATCH] D67264: [clangd] Collect location of macro definition in the ParsedAST

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:104
 // *definitions* in the preamble region of the main file).
 class CollectMainFileMacroExpansions : public PPCallbacks {
   const SourceManager &SM;
----------------
hokein wrote:
> The name doesn't fit into what it does anymore, maybe rename to `CollectMainFileMacroLocations`?
Agree, maybe even shorter - `CollectMainFileMacros`?



================
Comment at: clang-tools-extra/clangd/ParsedAST.h:125
+  /// preamble.
   /// Does not include expansions from inside other macro expansions.
   std::vector<SourceLocation> MainFileMacroExpLocs;
----------------
NIT: s/Does not include expansions/Does not include **locations**?

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


================
Comment at: clang-tools-extra/clangd/ParsedAST.h:126
   /// Does not include expansions from inside other macro expansions.
   std::vector<SourceLocation> MainFileMacroExpLocs;
   // Data, stored after parsing.
----------------
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.


================
Comment at: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp:234
+  namespace {
+    #define ^MACRO_ARGS(X, Y) X Y
     ^ID(int A);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67264