[PATCH] D83536: [clangd] Index refs to main-file symbols as well

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks, lgtm. mostly comments arounds tests, sorry for not looking at them before.



================
Comment at: clang-tools-extra/clangd/index/FileIndex.h:156
 /// Exposed to assist in unit tests.
-SlabTuple indexMainDecls(ParsedAST &AST);
+SlabTuple indexMainDecls(ParsedAST &AST, bool CollectMainFileRefs = true);
 
----------------
again default to false.


================
Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:194
+      /*ContextProvider=*/nullptr,
+      /*CollectMainFileRefs=*/true);
 
----------------
this requires it is own separate test, as this is not only testing for indexing two files case now.

I would suggest a minimal test with one header and one cc ref, running background index twice to make sure both configurations work as expected.


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:628
                                   HaveRanges(Main.ranges("macro")))));
-  // Symbols *only* in the main file:
-  // - (a, b) externally visible and should have refs.
-  // - (c, FUNC) externally invisible and had no refs collected.
-  auto MainSymbols =
-      TestTU::withHeaderCode(SymbolsOnlyInMainCode.code()).headerSymbols();
-  EXPECT_THAT(Refs, Contains(Pair(findSymbol(MainSymbols, "a").ID, _)));
-  EXPECT_THAT(Refs, Contains(Pair(findSymbol(MainSymbols, "b").ID, _)));
-  EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "c").ID, _))));
-  EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "FUNC").ID, _))));
+  // Symbols *only* in the main file (a, b, c) should have refs collected
+  // as well.
----------------
could you instead run twice ? once with mainfilerefs = false, and once with it set to true?


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:710
   } TestCases[] = {
-    {
-      R"cpp(
+      {
+          R"cpp(
----------------
these seem to be some unrelated formatting changes, please revert before landing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83536/new/

https://reviews.llvm.org/D83536