ilya-biryukov added inline comments.
================ Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:86 + // Add macro references. + for (const auto &IDToRefs : MacroRefsToIndex->MacroRefs) { + for (const auto &Range : IDToRefs.second) { ---------------- hokein wrote: > ilya-biryukov wrote: > > This is trying to emulate existing logic in `SymbolCollector::finish`. Is > > there a way we could share this? > > Would avoid creating extra copies of reference slabs and allow to keep the > > code in one place, rather than scattering it between `FileIndex.cpp` and > > `SymbolCollector.cpp`. Would also allow to keep `toURI` private, meaning we > > don't have to worry about naming it and the fact it's exposed in the public > > interface. > > > > One potential way to do this is to have an alternative version of > > `handleMacroOccurence`, which would fill `SymbolCollector::MacroRefs` > > directly and call this right after `indexTopLevelDecls`. > > Would that work? > +1 on the concern about performance (this is a hot function, we are paying an > extra cost of copying all refs, which should be avoided), and the layering of > `toURI`. > > > One potential way to do this is to have an alternative version of > > handleMacroOccurence, which would fill SymbolCollector::MacroRefs directly > > and call this right after indexTopLevelDecls. > > Would that work? > > the main problem is that at this point (parsing is finished), preprocessor > callback is not available, so we won't see macro references > (`SymbolCollector::handleMacroOccurence` will only receive macro definitions). > > I think two options: > - as mentioned above, add alternative version of `handleMacroOccurence` in > SymbolCollector, calling it **before** `indexTopLevelDecls` (because `finish` > is called in `indexTopLevelDecls` which we have no way to customize); > - or we could add a finish callback to the `SymbolCollector` which is called > in `SymbolCollector::finish`, and put this logic to the callback. > Thanks, good point, in my proposal we should populate macro references before calling `indexTopLevelDecls`. I would still suggest going with the first option, it seems simpler to me. But up to you, @usaxena95. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71406/new/ https://reviews.llvm.org/D71406 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits