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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits