kadircet added inline comments.
================ Comment at: clangd/index/SymbolCollector.h:130 + // The final spelling is calculated in finish(). + llvm::DenseMap<SymbolID, FileID> IncludeFiles; + // Indexed macros, to be erased if they turned out to be include guards. ---------------- sammccall wrote: > kadircet wrote: > > This is losing ability to store multiple header files. Is that intentional? > Careful reading of the code shows that ability never existed :-) > `addDeclaration` always creates a new Symbol, sometimes populates its > `IncludeHeaders`, and then replaces the existing symbol. We always find a > single decl of the symbol we prefer in the TU (though sometimes it takes us a > few attempts). > > Of course, I never read the code that carefully - I wrote this as a > `vector<pair<SymbolID, FileID>>` to "preserve" the ability to add multiple > headers... and then the tests failed :-) Ah, ok I see. Thanks for pointing this out! ================ Comment at: clangd/unittests/SymbolCollectorTests.cpp:1043 + int decl(); + #define MACRO + ---------------- sammccall wrote: > kadircet wrote: > > can you also add a case where we first see `MACRO` and then `decl`. > IIUC the idea is that macros are "eager-er" than decls so more likely to > break the caching. That makes sense. > > WDYT about just putting the macro first, so we *only* test the "hard" case. exactly, SGTM Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61442/new/ https://reviews.llvm.org/D61442 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
