[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf82346fd7391: [clangd] Avoid relations being overwritten in
a header shard (authored by nridge).
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/Base.h")] = "class Base {};";
+ FS.Files[testPath("root/A.cc")] = R"cpp(
+#include "Base.h"
+class A : public Base {};
+ )cpp";
+ FS.Files[testPath("root/B.cc")] = R"cpp(
+#include "Base.h"
+class B : public Base {};
+ )cpp";
+
+ llvm::StringMap Storage;
+ size_t CacheHits = 0;
+ MemoryShardStorage MSS(Storage, CacheHits);
+ OverlayCDB CDB(/*Base=*/nullptr);
+ BackgroundIndex Index(FS, CDB, [&](llvm::StringRef) { return },
+/*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/Base.h"));
+ EXPECT_NE(HeaderShard, nullptr);
+ SymbolID Base = findSymbol(*HeaderShard->Symbols, "Base").ID;
+
+ RelationsRequest Req;
+ Req.Subjects.insert(Base);
+ 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
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
nridge marked an inline comment as done.
nridge added inline comments.
Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:232
+TEST_F(BackgroundIndexTest, RelationsMultiFile) {
+ MockFS FS;
kadircet wrote:
> kadircet wrote:
> > nridge wrote:
> > > kadircet wrote:
> > > > do we still need this test?
> > > I think it's still useful, yes. If someone makes a change to the way
> > > relations are stored in the future, they could regress this use case
> > > without a test specifically for it.
> > this one was marked as resolved but i still don't see the reasoning. can we
> > test this in fileindextests instead?
> >
> > we already test the sharding logic, we need to test the merging logic now.
> > can we rather test it at FileSymbols layer directly? or is there something
> > extra that i am misisng?
> okay, i still think it is better to test on FileSymbols layer but not that
> important.
The reason I'd like to have a test at this layer is so that we have a test that
closely approximates the steps to reproduce: create files with certain contents
and inclusion relationships between them, index them, and perform a particular
query. I feel like internal abstractions like `FileSymbols` are liable to
change over time through refactorings, and so tests written against them are
less robust.
Note, I don't think using `BackgroundIndex` is important for the kind of test
I'd like; `FileIndex` would be fine too. However, I could not see how to use
`FileIndex` with files that contain `#include` statements (didn't see a way to
provide a `MockFS` or another way to get the includes to be resolved).
That said, I did try to write an additional test using `FileSymbols`, but I
found that:
* it only exercises the merging logic, not the sharding logic
* I couldn't get the test to fail even if I omit the merging logic of this
patch, because both `MemIndex` and `Dex` build a `DenseMap` for relations and
therefore end up implicitly deduplicating anyways
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87256/new/
https://reviews.llvm.org/D87256
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
nridge updated this revision to Diff 297482.
nridge added a comment.
Slightly simplified test (base class does not need to be a template to trigger
the issue)
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/Base.h")] = "class Base {};";
+ FS.Files[testPath("root/A.cc")] = R"cpp(
+#include "Base.h"
+class A : public Base {};
+ )cpp";
+ FS.Files[testPath("root/B.cc")] = R"cpp(
+#include "Base.h"
+class B : public Base {};
+ )cpp";
+
+ llvm::StringMap Storage;
+ size_t CacheHits = 0;
+ MemoryShardStorage MSS(Storage, CacheHits);
+ OverlayCDB CDB(/*Base=*/nullptr);
+ BackgroundIndex Index(FS, CDB, [&](llvm::StringRef) { return },
+/*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/Base.h"));
+ EXPECT_NE(HeaderShard, nullptr);
+ SymbolID Base = findSymbol(*HeaderShard->Symbols, "Base").ID;
+
+ RelationsRequest Req;
+ Req.Subjects.insert(Base);
+ 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
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thanks! (and sorry for taking too long on this one.)
Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:232
+TEST_F(BackgroundIndexTest, RelationsMultiFile) {
+ MockFS FS;
kadircet wrote:
> nridge wrote:
> > kadircet wrote:
> > > do we still need this test?
> > I think it's still useful, yes. If someone makes a change to the way
> > relations are stored in the future, they could regress this use case
> > without a test specifically for it.
> this one was marked as resolved but i still don't see the reasoning. can we
> test this in fileindextests instead?
>
> we already test the sharding logic, we need to test the merging logic now.
> can we rather test it at FileSymbols layer directly? or is there something
> extra that i am misisng?
okay, i still think it is better to test on FileSymbols layer but not that
important.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87256/new/
https://reviews.llvm.org/D87256
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
nridge added a comment.
Sorry, I had a response to that comment but accidentally left it as
unsubmitted...
Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:232
+TEST_F(BackgroundIndexTest, RelationsMultiFile) {
+ MockFS FS;
kadircet wrote:
> do we still need this test?
I think it's still useful, yes. If someone makes a change to the way relations
are stored in the future, they could regress this use case without a test
specifically for it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87256/new/
https://reviews.llvm.org/D87256
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:232
+TEST_F(BackgroundIndexTest, RelationsMultiFile) {
+ MockFS FS;
kadircet wrote:
> do we still need this test?
this one was marked as resolved but i still don't see the reasoning. can we
test this in fileindextests instead?
we already test the sharding logic, we need to test the merging logic now. can
we rather test it at FileSymbols layer directly? or is there something extra
that i am misisng?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87256/new/
https://reviews.llvm.org/D87256
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
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 class RAV {};";
+ FS.Files[testPath("root/A.cc")] = R"cpp(
+#include "RAV.h"
+class A : public RAV {};
+ )cpp";
+ FS.Files[testPath("root/B.cc")] = R"cpp(
+#include "RAV.h"
+class B : public RAV {};
+ )cpp";
+
+ llvm::StringMap Storage;
+ size_t CacheHits = 0;
+ MemoryShardStorage MSS(Storage, CacheHits);
+ OverlayCDB CDB(/*Base=*/nullptr);
+ BackgroundIndex Index(FS, CDB, [&](llvm::StringRef) { return },
+/*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
+ //
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:152
+ // Attribute relations to both their subject's and object's locations.
+ // See https://github.com/clangd/clangd/issues/510 for discussion of why.
if (Index.Relations) {
instead of (or in addition to) providing the link, can we give a short summary?
e.g.
```
Subject and/or Object files might be part of multiple TUs. In such cases there
will be a race
and last TU to write the shard will win and all the other relations will be
lost. We are storing
relations in both places, as we do for symbols, to reduce such races. Note that
they might still
happen if same translation unit produces different relations under different
configurations
but that's something clangd doesn't handle in general.
```
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:350
AllRelations.push_back(R);
+// Sort relations and remove duplicates that could arise due to
+// relations being stored in both the shards containing their
can you move this outside of the for loop, so that we do it only once.
Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:232
+TEST_F(BackgroundIndexTest, RelationsMultiFile) {
+ MockFS FS;
do we still need this test?
Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:396
SymbolID B = findSymbol(*ShardSource->Symbols, "B_CC").ID;
- EXPECT_THAT(*ShardHeader->Relations,
+ EXPECT_THAT(*ShardSource->Relations,
+ UnorderedElementsAre(Relation{A, RelationKind::BaseOf, B}));
s/ShardSource/ShardHeader
Comment at: clang-tools-extra/clangd/unittests/FileIndexTests.cpp:571
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 even though it's dangling
+B.insert(Relation{Sym3.ID, RelationKind::BaseOf, Sym1.ID});
maybe mention that `Sym1` belongs to `a.h` in the comment. `stored in a.h
(where Sym1 is stored) even tho the reference is dangling as Sym3 is unknown `
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87256/new/
https://reviews.llvm.org/D87256
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
nridge updated this revision to Diff 293045.
nridge added a comment.
Herald added a subscriber: mgrang.
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,12 @@
}
{
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 even though it's dangling
+B.insert(Relation{Sym3.ID, RelationKind::BaseOf, Sym1.ID});
IF.Relations.emplace(std::move(B).build());
}
@@ -605,7 +607,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 +621,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 class RAV {};";
+ FS.Files[testPath("root/A.cc")] = R"cpp(
+#include "RAV.h"
+class A : public RAV {};
+ )cpp";
+ FS.Files[testPath("root/B.cc")] = R"cpp(
+#include "RAV.h"
+class B : public RAV {};
+ )cpp";
+
+ llvm::StringMap Storage;
+ size_t CacheHits = 0;
+ MemoryShardStorage MSS(Storage, CacheHits);
+ OverlayCDB CDB(/*Base=*/nullptr);
+ BackgroundIndex Index(FS, CDB, [&](llvm::StringRef) { return },
+/*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
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
hokein added a comment. +@kadircet, he is tracking on this -- we had some discussion internally last week, but we don't reply on that thread yet (sorry). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87256/new/ https://reviews.llvm.org/D87256 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
nridge created this revision.
nridge added a reviewer: hokein.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous.
Herald added a project: clang.
nridge requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.
Fixes https://github.com/clangd/clangd/issues/510
Repository:
rG LLVM Github Monorepo
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,12 @@
}
{
RelationSlab::Builder B;
-// Should be stored in a.h
-B.insert(Relation{Sym1.ID, RelationKind::BaseOf, Sym2.ID});
// Should be stored in b.h
+B.insert(Relation{Sym1.ID, RelationKind::BaseOf, Sym2.ID});
+// Should be stored in a.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 even though it's dangling
+B.insert(Relation{Sym3.ID, RelationKind::BaseOf, Sym1.ID});
IF.Relations.emplace(std::move(B).build());
}
@@ -605,7 +607,8 @@
EXPECT_THAT(*Shard->Refs, IsEmpty());
EXPECT_THAT(
*Shard->Relations,
-UnorderedElementsAre(Relation{Sym1.ID, RelationKind::BaseOf, Sym2.ID}));
+UnorderedElementsAre(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 +620,7 @@
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}));
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 class RAV {};";
+ FS.Files[testPath("root/A.cc")] = R"cpp(
+#include "RAV.h"
+class A : public RAV {};
+ )cpp";
+ FS.Files[testPath("root/B.cc")] = R"cpp(
+#include "RAV.h"
+class B : public RAV {};
+ )cpp";
+
+ llvm::StringMap Storage;
+ size_t CacheHits = 0;
+ MemoryShardStorage MSS(Storage, CacheHits);
+ OverlayCDB CDB(/*Base=*/nullptr);
+ BackgroundIndex Index(FS, CDB, [&](llvm::StringRef) { return },
+/*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(
@@ -346,13 +389,13 @@
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)
+ // containing the definition of the object (B_CC)
SymbolID A = findSymbol(*ShardHeader->Symbols, "A_CC").ID;
SymbolID B = findSymbol(*ShardSource->Symbols, "B_CC").ID;
-
