[PATCH] D70489: [clangd] Add xref for macro to static index.

[Haojian Wu via Phabricator via cfe-commits]

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