[PATCH] D70008: [clangd] Store xref for Macros in ParsedAST.

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 marked an inline comment as done.
usaxena95 added inline comments.


================
Comment at: clang-tools-extra/clangd/CollectMacros.h:29
   std::vector<Range> Ranges;
+  llvm::DenseMap<SymbolID, std::set<Location>> MacroRefs;
 };
----------------
hokein wrote:
> I think the `Ranges` and `MacrosRefs` have a lot of duplications, it is wasteful to store a same range twice. We don't need the MainFilePath, the callers should know the corresponding file path for the AST. Something like `llvm::DenseMap<SymbolID, std::vector<Range>> Refs;` should be enough.
> 
> symbol id for macro generation is required the definition location, but only defined macros have it, we may need an extra `vector` to store ranges for undefined macros.
> 
> ```
> #ifndef abc // there is no macro info* for abc, so getSymbolID will fail, but we still want it, e.g. in semantic highlighting.
> #endif
> ``` 
> 
> There may be more tricky cases, it would be nice to have in the test.
> 
> ```
> #define abc 1
> #undef abc
> 
> #define abc 2
> #undef abc
> ```
- using `llvm::DenseMap<SymbolID, std::set<Range>> Refs;` as de-duplication is easier.
- Iterating on all the ranges is a pain. Let me know if you want to provide a different method to return all the ranges (sad part: lot of copying).
- I had multiple definitions of a macro in the test case, but I have shifted to its separate test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70008