[PATCH] D70489: [clangd] Add xref for macro to static index.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:374
+        Roles & static_cast<unsigned>(index::SymbolRole::Definition) ||
+        Roles & static_cast<unsigned>(index::SymbolRole::Reference)))
     return true;
----------------
I think we should avoid running the code below if this is a mere reference.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:532
+      const IdentifierInfo *II = MacroRef.first;
+      if (const auto *MI = PP->getMacroDefinition(II).getMacroInfo())
+        if (auto ID = getSymbolID(II->getName(), MI, PP->getSourceManager())) {
----------------
I'm curious of the behavior `getMacroDefinition` -- from its implementation, if `II` doesn't have macro definition, it just returns empty.

could you check whether it works at the below case (or even with a same macro name defined/undef multiple times)?

```
#define abc 1
#undef abc

// not at the EOF, I guess, II for `abc` doesn't have macro definition at this point because it has been undef?
```


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.h:76
     /// macro even if this is true.
     bool CollectMacro = false;
     /// Collect symbols local to main-files, such as static functions
----------------
This option is for macro symbols, I'd not use it for collecting macro references. I think the whether to collect macro references is judged by `RefFilter` and `RefsInHeaders`. 


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.h:154
   Options Opts;
   using DeclRef = std::pair<SourceLocation, index::SymbolRoleSet>;
   // Symbols referenced from the current TU, flushed on finish().
----------------
since we now use it for macros, the name is not valid any more, maybe rename it to `SymbolRef`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70489