nridge updated this revision to Diff 294577.
nridge marked 5 inline comments as done.
nridge added a comment.
Address review comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87256/new/
https://reviews.llvm.org/D87256
Files:
clang-tools-extra/clangd/index/FileIndex.cpp
clang-tools-extra/clangd/unittests/BackgroundIndexTests.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
@@ -547,6 +547,8 @@
Sym2.CanonicalDeclaration.FileURI = BHeaderUri.c_str();
Sym2.Definition.FileURI = BSourceUri.c_str();
+ auto Sym3 = symbol("3"); // not stored
+
IndexFileIn IF;
{
SymbolSlab::Builder B;
@@ -562,12 +564,13 @@
}
{
RelationSlab::Builder B;
- // Should be stored in a.h
+ // Should be stored in a.h and b.h
B.insert(Relation{Sym1.ID, RelationKind::BaseOf, Sym2.ID});
- // Should be stored in b.h
+ // Should be stored in a.h and b.h
B.insert(Relation{Sym2.ID, RelationKind::BaseOf, Sym1.ID});
- // Dangling relation should be dropped.
- B.insert(Relation{symbol("3").ID, RelationKind::BaseOf, Sym1.ID});
+ // Should be stored in a.h (where Sym1 is stored) even though
+ // the relation is dangling as Sym3 is unknown.
+ B.insert(Relation{Sym3.ID, RelationKind::BaseOf, Sym1.ID});
IF.Relations.emplace(std::move(B).build());
}
@@ -605,7 +608,9 @@
EXPECT_THAT(*Shard->Refs, IsEmpty());
EXPECT_THAT(
*Shard->Relations,
- UnorderedElementsAre(Relation{Sym1.ID, RelationKind::BaseOf, Sym2.ID}));
+ UnorderedElementsAre(Relation{Sym1.ID, RelationKind::BaseOf, Sym2.ID},
+ Relation{Sym2.ID, RelationKind::BaseOf, Sym1.ID},
+ Relation{Sym3.ID, RelationKind::BaseOf, Sym1.ID}));
ASSERT_THAT(Shard->Sources->keys(), UnorderedElementsAre(AHeaderUri));
EXPECT_THAT(Shard->Sources->lookup(AHeaderUri).DirectIncludes, IsEmpty());
EXPECT_TRUE(Shard->Cmd.hasValue());
@@ -617,7 +622,8 @@
EXPECT_THAT(*Shard->Refs, IsEmpty());
EXPECT_THAT(
*Shard->Relations,
- UnorderedElementsAre(Relation{Sym2.ID, RelationKind::BaseOf, Sym1.ID}));
+ UnorderedElementsAre(Relation{Sym1.ID, RelationKind::BaseOf, Sym2.ID},
+ Relation{Sym2.ID, RelationKind::BaseOf, Sym1.ID}));
ASSERT_THAT(Shard->Sources->keys(),
UnorderedElementsAre(BHeaderUri, AHeaderUri));
EXPECT_THAT(Shard->Sources->lookup(BHeaderUri).DirectIncludes,
Index: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
@@ -229,6 +229,49 @@
FileURI("unittest:///root/B.cc")}));
}
+TEST_F(BackgroundIndexTest, RelationsMultiFile) {
+ MockFS FS;
+ FS.Files[testPath("root/RAV.h")] = "template <typename T> class RAV {};";
+ FS.Files[testPath("root/A.cc")] = R"cpp(
+ #include "RAV.h"
+ class A : public RAV<A> {};
+ )cpp";
+ FS.Files[testPath("root/B.cc")] = R"cpp(
+ #include "RAV.h"
+ class B : public RAV<B> {};
+ )cpp";
+
+ llvm::StringMap<std::string> Storage;
+ size_t CacheHits = 0;
+ MemoryShardStorage MSS(Storage, CacheHits);
+ OverlayCDB CDB(/*Base=*/nullptr);
+ BackgroundIndex Index(FS, CDB, [&](llvm::StringRef) { return &MSS; },
+ /*Opts=*/{});
+
+ tooling::CompileCommand Cmd;
+ Cmd.Filename = testPath("root/A.cc");
+ Cmd.Directory = testPath("root");
+ Cmd.CommandLine = {"clang++", Cmd.Filename};
+ CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ ASSERT_TRUE(Index.blockUntilIdleForTest());
+
+ Cmd.Filename = testPath("root/B.cc");
+ Cmd.CommandLine = {"clang++", Cmd.Filename};
+ CDB.setCompileCommand(testPath("root/B.cc"), Cmd);
+ ASSERT_TRUE(Index.blockUntilIdleForTest());
+
+ auto HeaderShard = MSS.loadShard(testPath("root/RAV.h"));
+ EXPECT_NE(HeaderShard, nullptr);
+ SymbolID RAV = findSymbol(*HeaderShard->Symbols, "RAV").ID;
+
+ RelationsRequest Req;
+ Req.Subjects.insert(RAV);
+ Req.Predicate = RelationKind::BaseOf;
+ uint32_t Results = 0;
+ Index.relations(Req, [&](const SymbolID &, const Symbol &) { ++Results; });
+ EXPECT_EQ(Results, 2u);
+}
+
TEST_F(BackgroundIndexTest, MainFileRefs) {
MockFS FS;
FS.Files[testPath("root/A.h")] = R"cpp(
@@ -345,14 +388,15 @@
EXPECT_THAT(Ref.second,
UnorderedElementsAre(FileURI("unittest:///root/A.cc")));
- // The BaseOf relationship between A_CC and B_CC is stored in the file
- // containing the definition of the subject (A_CC)
+ // The BaseOf relationship between A_CC and B_CC is stored in both the file
+ // containing the definition of the subject (A_CC) and the file containing
+ // the definition of the object (B_CC).
SymbolID A = findSymbol(*ShardHeader->Symbols, "A_CC").ID;
SymbolID B = findSymbol(*ShardSource->Symbols, "B_CC").ID;
EXPECT_THAT(*ShardHeader->Relations,
UnorderedElementsAre(Relation{A, RelationKind::BaseOf, B}));
- // (and not in the file containing the definition of the object (B_CC)).
- EXPECT_EQ(ShardSource->Relations->size(), 0u);
+ EXPECT_THAT(*ShardSource->Relations,
+ UnorderedElementsAre(Relation{A, RelationKind::BaseOf, B}));
}
TEST_F(BackgroundIndexTest, DirectIncludesTest) {
Index: clang-tools-extra/clangd/index/FileIndex.cpp
===================================================================
--- clang-tools-extra/clangd/index/FileIndex.cpp
+++ clang-tools-extra/clangd/index/FileIndex.cpp
@@ -34,6 +34,7 @@
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
+#include <algorithm>
#include <memory>
#include <tuple>
#include <utility>
@@ -147,13 +148,21 @@
}
}
}
- // Attribute relations to the file declaraing their Subject as Object might
- // not have been indexed, see SymbolCollector::processRelations for details.
+ // The Subject and/or Object shards might be part of multiple TUs. In
+ // such cases there will be a race and the last TU to write the shard
+ // will win and all the other relations will be lost. To avoid this,
+ // we store relations in both shards. A race might still happen if the
+ // same translation unit produces different relations under different
+ // configurations, but that's something clangd doesn't handle in general.
if (Index.Relations) {
for (const auto &R : *Index.Relations) {
// FIXME: RelationSlab shouldn't contain dangling relations.
- if (auto *File = SymbolIDToFile.lookup(R.Subject))
- File->Relations.insert(&R);
+ FileShard *SubjectFile = SymbolIDToFile.lookup(R.Subject);
+ FileShard *ObjectFile = SymbolIDToFile.lookup(R.Object);
+ if (SubjectFile)
+ SubjectFile->Relations.insert(&R);
+ if (ObjectFile && ObjectFile != SubjectFile)
+ ObjectFile->Relations.insert(&R);
}
}
// Store only the direct includes of a file in a shard.
@@ -343,6 +352,12 @@
for (const auto &R : *RelationSlab)
AllRelations.push_back(R);
}
+ // Sort relations and remove duplicates that could arise due to
+ // relations being stored in both the shards containing their
+ // subject and object.
+ llvm::sort(AllRelations);
+ AllRelations.erase(std::unique(AllRelations.begin(), AllRelations.end()),
+ AllRelations.end());
size_t StorageSize =
RefsStorage.size() * sizeof(Ref) + SymsStorage.size() * sizeof(Symbol);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
