hokein created this revision.
hokein added a reviewer: kadircet.
Herald added subscribers: arphaman, jkorous, MaskRay, ilya-biryukov.
Herald added a project: clang.
Previously, we randomly pick one main file symbol in dynamic index, we
may loose the ideal symbol (with definition location) in the index.
It fixes the issue where sometimes we fail to go to the symbol definition, see:
1. call go-to-decl on Foo in Foo.cpp
2. jump to Foo.h, call go-to-def on Foo in Foo.h
we can't go back to Foo.cpp -- because we open Foo.cpp, Foo.h in clangd, both
files have Foo symbol (one with def&decl, one with decl only), we randomely
choose one.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D63425
Files:
clang-tools-extra/clangd/index/FileIndex.cpp
clang-tools-extra/clangd/unittests/FileIndexTests.cpp
Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/FileIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp
@@ -46,6 +46,7 @@
}
MATCHER_P(QName, N, "") { return (arg.Scope + arg.Name).str() == N; }
MATCHER_P(NumReferences, N, "") { return arg.References == N; }
+MATCHER_P(hasOrign, O, "") { return bool(arg.Origin & O); }
namespace clang {
namespace clangd {
@@ -367,6 +368,27 @@
RefsAre({RefRange(Main.range())}));
}
+TEST(FileIndexTest, MergeMainFileSymbols) {
+ const char* CommonHeader = "void foo();";
+ TestTU Header = TestTU::withCode(CommonHeader);
+ TestTU Cpp = TestTU::withCode("void foo() {}");
+ Cpp.Filename = "foo.cpp";
+ Cpp.HeaderFilename = "foo.h";
+ Cpp.HeaderCode = CommonHeader;
+
+ FileIndex Index;
+ auto HeaderAST = Header.build();
+ auto CppAST = Cpp.build();
+ Index.updateMain(testPath("foo.h"), HeaderAST);
+ Index.updateMain(testPath("foo.cpp"), CppAST);
+
+ auto Symbols = runFuzzyFind(Index, "");
+ // Check foo is merged, foo in Cpp wins (as we see the definition there).
+ EXPECT_THAT(Symbols, ElementsAre(AllOf(DeclURI("unittest:///foo.h"),
+ DefURI("unittest:///foo.cpp"),
+ hasOrign(SymbolOrigin::Merge))));
+}
+
TEST(FileSymbolsTest, CountReferencesNoRefSlabs) {
FileSymbols FS;
FS.update("f1", numSlab(1, 3), nullptr, true);
Index: clang-tools-extra/clangd/index/FileIndex.cpp
===================================================================
--- clang-tools-extra/clangd/index/FileIndex.cpp
+++ clang-tools-extra/clangd/index/FileIndex.cpp
@@ -236,7 +236,7 @@
llvm::make_unique<RefSlab>(std::move(Contents.second)),
/*CountReferences=*/true);
MainFileIndex.reset(
- MainFileSymbols.buildIndex(IndexType::Light,
DuplicateHandling::PickOne));
+ MainFileSymbols.buildIndex(IndexType::Light, DuplicateHandling::Merge));
}
} // namespace clangd
Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/FileIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp
@@ -46,6 +46,7 @@
}
MATCHER_P(QName, N, "") { return (arg.Scope + arg.Name).str() == N; }
MATCHER_P(NumReferences, N, "") { return arg.References == N; }
+MATCHER_P(hasOrign, O, "") { return bool(arg.Origin & O); }
namespace clang {
namespace clangd {
@@ -367,6 +368,27 @@
RefsAre({RefRange(Main.range())}));
}
+TEST(FileIndexTest, MergeMainFileSymbols) {
+ const char* CommonHeader = "void foo();";
+ TestTU Header = TestTU::withCode(CommonHeader);
+ TestTU Cpp = TestTU::withCode("void foo() {}");
+ Cpp.Filename = "foo.cpp";
+ Cpp.HeaderFilename = "foo.h";
+ Cpp.HeaderCode = CommonHeader;
+
+ FileIndex Index;
+ auto HeaderAST = Header.build();
+ auto CppAST = Cpp.build();
+ Index.updateMain(testPath("foo.h"), HeaderAST);
+ Index.updateMain(testPath("foo.cpp"), CppAST);
+
+ auto Symbols = runFuzzyFind(Index, "");
+ // Check foo is merged, foo in Cpp wins (as we see the definition there).
+ EXPECT_THAT(Symbols, ElementsAre(AllOf(DeclURI("unittest:///foo.h"),
+ DefURI("unittest:///foo.cpp"),
+ hasOrign(SymbolOrigin::Merge))));
+}
+
TEST(FileSymbolsTest, CountReferencesNoRefSlabs) {
FileSymbols FS;
FS.update("f1", numSlab(1, 3), nullptr, true);
Index: clang-tools-extra/clangd/index/FileIndex.cpp
===================================================================
--- clang-tools-extra/clangd/index/FileIndex.cpp
+++ clang-tools-extra/clangd/index/FileIndex.cpp
@@ -236,7 +236,7 @@
llvm::make_unique<RefSlab>(std::move(Contents.second)),
/*CountReferences=*/true);
MainFileIndex.reset(
- MainFileSymbols.buildIndex(IndexType::Light, DuplicateHandling::PickOne));
+ MainFileSymbols.buildIndex(IndexType::Light, DuplicateHandling::Merge));
}
} // namespace clangd
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
