[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/index/FileIndex.cpp:84
+    Merged.insert(Macro);
+  for (const auto &Sym : Collector.takeSymbols())
+    Merged.insert(Sym);
----------------
ioeric wrote:
> ilya-biryukov wrote:
> > Why make an extra copy of the symbols? Why not add macros to the same builder used in collector?
> `index::indexTopLevelDecls` will re-`initialize` the collector. This is safe with the current implementation, but I can imagine it goes wrong when we do more cleanup in the initialization.
Sorry, missed the comment about it.
And if we do this after `indexTopLevelDecls`, than we'll be calling after the `finish` call. **Sigh**...

Doing an extra copy of the symbols here is wasteful and makes the code more complicated than it should be.

We have alternatives that could fix this:
1. Move `IndexMacro` logic into `index::indexTopLevelDecls`
2. Move macro collection to `SymbolCollector::finish()`
3. Extract a function that creates a symbol for macro definition, so we don't need to call `handleMacroOccurence` and add can add extra macro symbols after `SymbolCollector` finishes.


(1) seems to be the cleanest option, WDYT?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078