hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.
thanks, looks good with a few nits. I think the benchmark data doesn't correct
any more with the latest patch, we don't increase number of symbols.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:368
+
+ // Do not store references to main-file symbols.
+ if ((static_cast<unsigned>(Opts.RefFilter) & Roles) && !IsMainFileSymbol &&
----------------
nit: symbols => macros, and other places as well.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:552
+ for (const auto &LocAndRole : MacroRef.second) {
+ CollectRef(ID, LocAndRole);
+ }
----------------
nit: I'd just inline the `ID` here.
no `{}` needed for single-body for statement.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:559
if (auto ID = getSymbolID(It.first)) {
for (const auto &LocAndRole : It.second) {
+ CollectRef(*ID, LocAndRole);
----------------
nit: remove the `{}`.
================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:672
+ HaveRanges(Header.ranges("bar")))));
+ EXPECT_THAT(Refs, Contains(Pair(_, HaveRanges(Header.ranges("ud1")))));
+ EXPECT_THAT(Refs, Contains(Pair(_, HaveRanges(Header.ranges("ud2")))));
----------------
could you add a comment why we use `_` here rather than the macro name?
================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:701
+ runSymbolCollector(Header.code(), Main.code());
+ auto HeaderSymbols = TestTU::withHeaderCode(Header.code()).headerSymbols();
+ EXPECT_THAT(Refs, IsEmpty());
----------------
nit: this variable is not used.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70489/new/
https://reviews.llvm.org/D70489
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
