kadircet updated this revision to Diff 333956.
kadircet marked 3 inline comments as done.
kadircet added a comment.
- Add comments to the unittest to explain why we chose incorrect behaviour.
- Extract authorization logic to a helper.
- Count dropped symbols separately in fuzzyfind tracer stats.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98538/new/
https://reviews.llvm.org/D98538
Files:
clang-tools-extra/clangd/index/Index.h
clang-tools-extra/clangd/index/Merge.cpp
clang-tools-extra/clangd/unittests/IndexTests.cpp
Index: clang-tools-extra/clangd/unittests/IndexTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IndexTests.cpp
+++ clang-tools-extra/clangd/unittests/IndexTests.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "Annotations.h"
+#include "SyncAPI.h"
#include "TestIndex.h"
#include "TestTU.h"
#include "index/FileIndex.h"
@@ -17,6 +18,7 @@
#include "clang/Index/IndexSymbol.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
+#include <utility>
using ::testing::_;
using ::testing::AllOf;
@@ -312,16 +314,28 @@
AST = Test.build();
DynamicIndex.updateMain(testPath(Test.Filename), AST);
- // Merged index should not return the symbol definition if this definition
- // location is inside a file from the dynamic index.
+ // Even though the definition is actually deleted in the newer version of the
+ // file, we still chose to merge with information coming from static index.
+ // This seems wrong, but is generic behavior we want for e.g. include headers
+ // which are always missing from the dynamic index
LookupRequest LookupReq;
LookupReq.IDs = {Foo.ID};
unsigned SymbolCounter = 0;
Merge.lookup(LookupReq, [&](const Symbol &Sym) {
++SymbolCounter;
- EXPECT_FALSE(Sym.Definition);
+ EXPECT_TRUE(Sym.Definition);
});
EXPECT_EQ(SymbolCounter, 1u);
+
+ // Drop the symbol completely.
+ Test.Code = "class Bar {};";
+ AST = Test.build();
+ DynamicIndex.updateMain(testPath(Test.Filename), AST);
+
+ // Now we don't expect to see the symbol at all.
+ SymbolCounter = 0;
+ Merge.lookup(LookupReq, [&](const Symbol &Sym) { ++SymbolCounter; });
+ EXPECT_EQ(SymbolCounter, 0u);
}
TEST(MergeIndexTest, FuzzyFind) {
@@ -585,6 +599,44 @@
IncludeHeaderWithRef("new", 1u)));
}
+TEST(MergeIndexTest, IncludeHeadersMerged) {
+ auto S = symbol("Z");
+ S.Definition.FileURI = "unittest:///foo.cc";
+
+ SymbolSlab::Builder DynB;
+ S.IncludeHeaders.clear();
+ DynB.insert(S);
+ SymbolSlab DynSymbols = std::move(DynB).build();
+ RefSlab DynRefs;
+ auto DynSize = DynSymbols.bytes() + DynRefs.bytes();
+ auto DynData = std::make_pair(std::move(DynSymbols), std::move(DynRefs));
+ llvm::StringSet<> DynFiles = {S.Definition.FileURI};
+ MemIndex DynIndex(std::move(DynData.first), std::move(DynData.second),
+ RelationSlab(), std::move(DynFiles), IndexContents::Symbols,
+ std::move(DynData), DynSize);
+
+ SymbolSlab::Builder StaticB;
+ S.IncludeHeaders.push_back({"<header>", 0});
+ StaticB.insert(S);
+ auto StaticIndex =
+ MemIndex::build(std::move(StaticB).build(), RefSlab(), RelationSlab());
+ MergedIndex Merge(&DynIndex, StaticIndex.get());
+
+ EXPECT_THAT(runFuzzyFind(Merge, S.Name),
+ ElementsAre(testing::Field(
+ &Symbol::IncludeHeaders,
+ ElementsAre(IncludeHeaderWithRef("<header>", 0u)))));
+
+ LookupRequest Req;
+ Req.IDs = {S.ID};
+ std::string IncludeHeader;
+ Merge.lookup(Req, [&](const Symbol &S) {
+ EXPECT_TRUE(IncludeHeader.empty());
+ ASSERT_EQ(S.IncludeHeaders.size(), 1u);
+ IncludeHeader = S.IncludeHeaders.front().IncludeHeader.str();
+ });
+ EXPECT_EQ(IncludeHeader, "<header>");
+}
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/index/Merge.cpp
===================================================================
--- clang-tools-extra/clangd/index/Merge.cpp
+++ clang-tools-extra/clangd/index/Merge.cpp
@@ -22,6 +22,19 @@
namespace clang {
namespace clangd {
+namespace {
+
+// Returns true if file defining/declaring \p S is covered by \p Index.
+bool isIndexAuthoritative(const SymbolIndex::IndexedFiles &Index,
+ const Symbol &S) {
+ // We expect the definition to see the canonical declaration, so it seems to
+ // be enough to check only the definition if it exists.
+ const char *OwningFile =
+ S.Definition ? S.Definition.FileURI : S.CanonicalDeclaration.FileURI;
+ return (Index(OwningFile) & IndexContents::Symbols) != IndexContents::None;
+}
+} // namespace
+
bool MergedIndex::fuzzyFind(
const FuzzyFindRequest &Req,
llvm::function_ref<void(const Symbol &)> Callback) const {
@@ -37,36 +50,44 @@
unsigned DynamicCount = 0;
unsigned StaticCount = 0;
unsigned MergedCount = 0;
+ // Number of results ignored due to staleness.
+ unsigned StaticDropped = 0;
More |= Dynamic->fuzzyFind(Req, [&](const Symbol &S) {
++DynamicCount;
DynB.insert(S);
});
SymbolSlab Dyn = std::move(DynB).build();
- llvm::DenseSet<SymbolID> SeenDynamicSymbols;
+ llvm::DenseSet<SymbolID> ReportedDynSymbols;
{
auto DynamicContainsFile = Dynamic->indexedFiles();
More |= Static->fuzzyFind(Req, [&](const Symbol &S) {
- // We expect the definition to see the canonical declaration, so it seems
- // to be enough to check only the definition if it exists.
- if ((DynamicContainsFile(S.Definition ? S.Definition.FileURI
- : S.CanonicalDeclaration.FileURI) &
- IndexContents::Symbols) != IndexContents::None)
- return;
- auto DynS = Dyn.find(S.ID);
++StaticCount;
- if (DynS == Dyn.end())
- return Callback(S);
- ++MergedCount;
- SeenDynamicSymbols.insert(S.ID);
- Callback(mergeSymbol(*DynS, S));
+ auto DynS = Dyn.find(S.ID);
+ // If symbol also exist in the dynamic index, just merge and report.
+ if (DynS != Dyn.end()) {
+ ++MergedCount;
+ ReportedDynSymbols.insert(S.ID);
+ return Callback(mergeSymbol(*DynS, S));
+ }
+
+ // Otherwise, if the dynamic index owns the symbol's file, it means static
+ // index is stale just drop the symbol.
+ if (isIndexAuthoritative(DynamicContainsFile, S)) {
+ ++StaticDropped;
+ return;
+ }
+
+ // If not just report the symbol from static index as is.
+ return Callback(S);
});
}
SPAN_ATTACH(Tracer, "dynamic", DynamicCount);
SPAN_ATTACH(Tracer, "static", StaticCount);
+ SPAN_ATTACH(Tracer, "static_dropped", StaticDropped);
SPAN_ATTACH(Tracer, "merged", MergedCount);
for (const Symbol &S : Dyn)
- if (!SeenDynamicSymbols.count(S.ID))
+ if (!ReportedDynSymbols.count(S.ID))
Callback(S);
return More;
}
@@ -83,18 +104,21 @@
{
auto DynamicContainsFile = Dynamic->indexedFiles();
Static->lookup(Req, [&](const Symbol &S) {
- // We expect the definition to see the canonical declaration, so it seems
- // to be enough to check only the definition if it exists.
- if ((DynamicContainsFile(S.Definition ? S.Definition.FileURI
- : S.CanonicalDeclaration.FileURI) &
- IndexContents::Symbols) != IndexContents::None)
+ // If we've seen the symbol before, just merge.
+ if (const Symbol *Sym = B.find(S.ID)) {
+ RemainingIDs.erase(S.ID);
+ return Callback(mergeSymbol(*Sym, S));
+ }
+
+ // If symbol is missing in dynamic index, and dynamic index owns the
+ // symbol's file. Static index is stale, just drop the symbol.
+ if (isIndexAuthoritative(DynamicContainsFile, S))
return;
- const Symbol *Sym = B.find(S.ID);
+
+ // Dynamic index doesn't know about this file, just use the symbol from
+ // static index.
RemainingIDs.erase(S.ID);
- if (!Sym)
- Callback(S);
- else
- Callback(mergeSymbol(*Sym, S));
+ Callback(S);
});
}
for (const auto &ID : RemainingIDs)
Index: clang-tools-extra/clangd/index/Index.h
===================================================================
--- clang-tools-extra/clangd/index/Index.h
+++ clang-tools-extra/clangd/index/Index.h
@@ -149,8 +149,9 @@
/// Returns function which checks if the specified file was used to build this
/// index or not. The function must only be called while the index is alive.
- virtual llvm::unique_function<IndexContents(llvm::StringRef) const>
- indexedFiles() const = 0;
+ using IndexedFiles =
+ llvm::unique_function<IndexContents(llvm::StringRef) const>;
+ virtual IndexedFiles indexedFiles() const = 0;
/// Returns estimated size of index (in bytes).
virtual size_t estimateMemoryUsage() const = 0;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
