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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
