ioeric added a comment. In https://reviews.llvm.org/D52078#1234667, @ilya-biryukov wrote:
> ~1% increase in memory usage seems totally fine. Actually surprised it's that > small. Tested on a larger file: it's ~5% for `ClangdServer.cpp`. I think it's still worth it for the speedup. ================ Comment at: clangd/index/FileIndex.cpp:84 + Merged.insert(Macro); + for (const auto &Sym : Collector.takeSymbols()) + Merged.insert(Sym); ---------------- ilya-biryukov wrote: > 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? (1) sounds good. D52098 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
