ilya-biryukov created this revision.
ilya-biryukov added reviewers: hokein, ioeric.
Herald added subscribers: arphaman, jkorous, MaskRay.
It was previously only indexing the preamble decls. The new
implementation will index both the preamble and the main AST and
report both sets of symbols, preferring the ones from the main AST
whenever the symbol is present in both.
The symbols in the main AST slab always store all information
available in the preamble symbols, possibly adding more,
e.g. definition locations.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50889
Files:
clangd/ClangdServer.cpp
clangd/index/FileIndex.cpp
clangd/index/FileIndex.h
clangd/index/Index.h
clangd/index/SymbolCollector.h
unittests/clangd/FileIndexTests.cpp
unittests/clangd/TestTU.cpp
Index: unittests/clangd/TestTU.cpp
===================================================================
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -45,7 +45,7 @@
SymbolSlab TestTU::headerSymbols() const {
auto AST = build();
- return indexAST(AST.getASTContext(), AST.getPreprocessorPtr());
+ return indexPreambleAST(AST.getASTContext(), AST.getPreprocessorPtr());
}
std::unique_ptr<SymbolIndex> TestTU::index() const {
Index: unittests/clangd/FileIndexTests.cpp
===================================================================
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -51,28 +51,28 @@
FileSymbols FS;
EXPECT_THAT(getSymbolNames(*FS.allSymbols()), UnorderedElementsAre());
- FS.update("f1", numSlab(1, 3));
+ FS.updatePreamble("f1", numSlab(1, 3));
EXPECT_THAT(getSymbolNames(*FS.allSymbols()),
UnorderedElementsAre("1", "2", "3"));
}
TEST(FileSymbolsTest, Overlap) {
FileSymbols FS;
- FS.update("f1", numSlab(1, 3));
- FS.update("f2", numSlab(3, 5));
+ FS.updatePreamble("f1", numSlab(1, 3));
+ FS.updatePreamble("f2", numSlab(3, 5));
EXPECT_THAT(getSymbolNames(*FS.allSymbols()),
UnorderedElementsAre("1", "2", "3", "3", "4", "5"));
}
TEST(FileSymbolsTest, SnapshotAliveAfterRemove) {
FileSymbols FS;
- FS.update("f1", numSlab(1, 3));
+ FS.updatePreamble("f1", numSlab(1, 3));
auto Symbols = FS.allSymbols();
EXPECT_THAT(getSymbolNames(*Symbols), UnorderedElementsAre("1", "2", "3"));
- FS.update("f1", nullptr);
+ FS.updatePreamble("f1", nullptr);
EXPECT_THAT(getSymbolNames(*FS.allSymbols()), UnorderedElementsAre());
EXPECT_THAT(getSymbolNames(*Symbols), UnorderedElementsAre("1", "2", "3"));
}
@@ -93,7 +93,7 @@
File.HeaderFilename = (Basename + ".h").str();
File.HeaderCode = Code;
auto AST = File.build();
- M.update(File.Filename, &AST.getASTContext(), AST.getPreprocessorPtr());
+ M.updatePreamble(File.Filename, &AST.getASTContext(), AST.getPreprocessorPtr());
}
TEST(FileIndexTest, CustomizedURIScheme) {
@@ -149,13 +149,13 @@
Req.Scopes = {"ns::"};
EXPECT_THAT(match(M, Req), UnorderedElementsAre("ns::f", "ns::X"));
- M.update("f1.cpp", nullptr, nullptr);
+ M.updatePreamble("f1.cpp", nullptr, nullptr);
EXPECT_THAT(match(M, Req), UnorderedElementsAre());
}
TEST(FileIndexTest, RemoveNonExisting) {
FileIndex M;
- M.update("no.cpp", nullptr, nullptr);
+ M.updatePreamble("no.cpp", nullptr, nullptr);
EXPECT_THAT(match(M, FuzzyFindRequest()), UnorderedElementsAre());
}
@@ -255,7 +255,7 @@
std::shared_ptr<Preprocessor> PP) {
EXPECT_FALSE(IndexUpdated) << "Expected only a single index update";
IndexUpdated = true;
- Index.update(FilePath, &Ctx, std::move(PP));
+ Index.updatePreamble(FilePath, &Ctx, std::move(PP));
});
ASSERT_TRUE(IndexUpdated);
Index: clangd/index/SymbolCollector.h
===================================================================
--- clangd/index/SymbolCollector.h
+++ clangd/index/SymbolCollector.h
@@ -85,6 +85,7 @@
SourceLocation Loc) override;
SymbolSlab takeSymbols() { return std::move(Symbols).build(); }
+ SymbolSlab::Builder takeBuilder() { return std::move(Symbols); }
void finish() override;
Index: clangd/index/Index.h
===================================================================
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -267,6 +267,8 @@
return I == SymbolIndex.end() ? nullptr : &Symbols[I->second];
}
+ llvm::ArrayRef<Symbol> symbols() const { return Symbols; }
+
// Consumes the builder to finalize the slab.
SymbolSlab build() &&;
Index: clangd/index/FileIndex.h
===================================================================
--- clangd/index/FileIndex.h
+++ clangd/index/FileIndex.h
@@ -41,16 +41,23 @@
public:
/// \brief Updates all symbols in a file. If \p Slab is nullptr, symbols for
/// \p Path will be removed.
- void update(PathRef Path, std::unique_ptr<SymbolSlab> Slab);
+ void updatePreamble(PathRef Path, std::unique_ptr<SymbolSlab> Slab);
+ void updateMainFile(PathRef Path, SymbolSlab::Builder Builder);
// The shared_ptr keeps the symbols alive
std::shared_ptr<std::vector<const Symbol *>> allSymbols();
private:
mutable std::mutex Mutex;
+ struct FileSlabs {
+ /// Contains symbols in the preamble.
+ std::shared_ptr<SymbolSlab> Preamble;
+ /// Contains symbol in the main file.
+ std::shared_ptr<SymbolSlab> MainFile;
+ };
/// \brief Stores the latest snapshots for all active files.
- llvm::StringMap<std::shared_ptr<SymbolSlab>> FileToSlabs;
+ llvm::StringMap<FileSlabs> FileToSlabs;
};
/// \brief This manages symbls from files and an in-memory index on all symbols.
@@ -64,7 +71,9 @@
/// nullptr, this removes all symbols in the file.
/// If \p AST is not null, \p PP cannot be null and it should be the
/// preprocessor that was used to build \p AST.
- void update(PathRef Path, ASTContext *AST, std::shared_ptr<Preprocessor> PP);
+ void updatePreamble(PathRef Path, ASTContext *AST, std::shared_ptr<Preprocessor> PP);
+ /// FIXME(ibiryukov): add comments before submitting this.
+ void updateMainFile(PathRef Path, ParsedAST &AST);
bool
fuzzyFind(const FuzzyFindRequest &Req,
@@ -86,8 +95,8 @@
/// Retrieves namespace and class level symbols in \p AST.
/// Exposed to assist in unit tests.
/// If URISchemes is empty, the default schemes in SymbolCollector will be used.
-SymbolSlab indexAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
- llvm::ArrayRef<std::string> URISchemes = {});
+SymbolSlab indexPreambleAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
+ llvm::ArrayRef<std::string> URISchemes = {});
} // namespace clangd
} // namespace clang
Index: clangd/index/FileIndex.cpp
===================================================================
--- clangd/index/FileIndex.cpp
+++ clangd/index/FileIndex.cpp
@@ -8,30 +8,55 @@
//===----------------------------------------------------------------------===//
#include "FileIndex.h"
-#include "SymbolCollector.h"
#include "../Logger.h"
+#include "Merge.h"
+#include "SymbolCollector.h"
#include "clang/Index/IndexingAction.h"
#include "clang/Lex/Preprocessor.h"
namespace clang {
namespace clangd {
-SymbolSlab indexAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
- llvm::ArrayRef<std::string> URISchemes) {
+namespace {
+SymbolCollector setupSymbolCollector(std::shared_ptr<Preprocessor> PP,
+ llvm::ArrayRef<std::string> URISchemes) {
SymbolCollector::Options CollectorOpts;
- // FIXME(ioeric): we might also want to collect include headers. We would need
- // to make sure all includes are canonicalized (with CanonicalIncludes), which
- // is not trivial given the current way of collecting symbols: we only have
- // AST at this point, but we also need preprocessor callbacks (e.g.
- // CommentHandler for IWYU pragma) to canonicalize includes.
+ // FIXME(ioeric): we might also want to collect include headers. We would
+ // need to make sure all includes are canonicalized (with
+ // CanonicalIncludes), which is not trivial given the current way of
+ // collecting symbols: we only have AST at this point, but we also need
+ // preprocessor callbacks (e.g. CommentHandler for IWYU pragma) to
+ // canonicalize includes.
CollectorOpts.CollectIncludePath = false;
CollectorOpts.CountReferences = false;
if (!URISchemes.empty())
CollectorOpts.URISchemes = URISchemes;
CollectorOpts.Origin = SymbolOrigin::Dynamic;
SymbolCollector Collector(std::move(CollectorOpts));
Collector.setPreprocessor(PP);
+ return Collector;
+}
+
+SymbolSlab::Builder indexMainAST(ASTContext &AST,
+ std::shared_ptr<Preprocessor> PP,
+ llvm::ArrayRef<const Decl *> TopLevelDecls,
+ llvm::ArrayRef<std::string> URISchemes) {
+ SymbolCollector Collector = setupSymbolCollector(PP, URISchemes);
+ index::IndexingOptions IndexOpts;
+ // We only need declarations, because we don't count references.
+ IndexOpts.SystemSymbolFilter =
+ index::IndexingOptions::SystemSymbolFilterKind::DeclarationsOnly;
+ IndexOpts.IndexFunctionLocals = false;
+
+ index::indexTopLevelDecls(AST, TopLevelDecls, Collector, IndexOpts);
+ return Collector.takeBuilder();
+}
+} // namespace
+
+SymbolSlab indexPreambleAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
+ llvm::ArrayRef<std::string> URISchemes) {
+ SymbolCollector Collector = setupSymbolCollector(PP, URISchemes);
index::IndexingOptions IndexOpts;
// We only need declarations, because we don't count references.
IndexOpts.SystemSymbolFilter =
@@ -49,17 +74,39 @@
FileIndex::FileIndex(std::vector<std::string> URISchemes)
: URISchemes(std::move(URISchemes)) {}
-void FileSymbols::update(PathRef Path, std::unique_ptr<SymbolSlab> Slab) {
+void FileSymbols::updatePreamble(PathRef Path,
+ std::unique_ptr<SymbolSlab> Slab) {
std::lock_guard<std::mutex> Lock(Mutex);
if (!Slab)
FileToSlabs.erase(Path);
else
- FileToSlabs[Path] = std::move(Slab);
+ FileToSlabs[Path].Preamble = std::move(Slab);
+}
+
+void FileSymbols::updateMainFile(PathRef Path, SymbolSlab::Builder Builder) {
+ // FIXME(ibiryukov): most of this work should be done outside the lock.
+ std::lock_guard<std::mutex> Lock(Mutex);
+ auto &FileSlabs = FileToSlabs[Path];
+ if (FileSlabs.Preamble) {
+ // Merge all missing data from preamble. We need this because documentation
+ // may be missing in symbols with definition in the main file and
+ // declaration in the preamble.
+ for (const Symbol &MainFileSym : Builder.symbols()) {
+ auto PreambleSymIt = FileSlabs.Preamble->find(MainFileSym.ID);
+ if (PreambleSymIt == FileSlabs.Preamble->end())
+ continue;
+
+ Symbol::Details Scratch;
+ Builder.insert(mergeSymbol(MainFileSym, *PreambleSymIt, &Scratch));
+ }
+ }
+
+ FileSlabs.MainFile = std::make_shared<SymbolSlab>(std::move(Builder).build());
}
std::shared_ptr<std::vector<const Symbol *>> FileSymbols::allSymbols() {
- // The snapshot manages life time of symbol slabs and provides pointers of all
- // symbols in all slabs.
+ // The snapshot manages life time of symbol slabs and provides pointers of
+ // all symbols in all slabs.
struct Snapshot {
std::vector<const Symbol *> Pointers;
std::vector<std::shared_ptr<SymbolSlab>> KeepAlive;
@@ -69,31 +116,58 @@
std::lock_guard<std::mutex> Lock(Mutex);
for (const auto &FileAndSlab : FileToSlabs) {
- Snap->KeepAlive.push_back(FileAndSlab.second);
- for (const auto &Iter : *FileAndSlab.second)
- Snap->Pointers.push_back(&Iter);
+ // Add symbol from the main file.
+ llvm::DenseSet<SymbolID> MainFileSymbols;
+ if (FileAndSlab.second.MainFile) {
+ Snap->KeepAlive.push_back(FileAndSlab.second.MainFile);
+ for (const auto &Iter : *FileAndSlab.second.MainFile) {
+ Snap->Pointers.push_back(&Iter);
+ MainFileSymbols.insert(Iter.ID);
+ }
+ }
+
+ // Add symbols from preamble.
+ if (FileAndSlab.second.Preamble) {
+ Snap->KeepAlive.push_back(FileAndSlab.second.Preamble);
+ for (const auto &Iter : *FileAndSlab.second.Preamble) {
+ /// Prefer symbols from the main file, we ensure they have strictly
+ /// more information than the matching ones from preamble.
+ if (MainFileSymbols.find(Iter.ID) != MainFileSymbols.end())
+ continue;
+ Snap->Pointers.push_back(&Iter);
+ }
+ }
}
}
auto *Pointers = &Snap->Pointers;
// Use aliasing constructor to keep the snapshot alive along with the
// pointers.
return {std::move(Snap), Pointers};
}
-void FileIndex::update(PathRef Path, ASTContext *AST,
- std::shared_ptr<Preprocessor> PP) {
+void FileIndex::updatePreamble(PathRef Path, ASTContext *AST,
+ std::shared_ptr<Preprocessor> PP) {
if (!AST) {
- FSymbols.update(Path, nullptr);
+ FSymbols.updatePreamble(Path, nullptr);
} else {
assert(PP);
auto Slab = llvm::make_unique<SymbolSlab>();
- *Slab = indexAST(*AST, PP, URISchemes);
- FSymbols.update(Path, std::move(Slab));
+ *Slab = indexPreambleAST(*AST, PP, URISchemes);
+ FSymbols.updatePreamble(Path, std::move(Slab));
}
auto Symbols = FSymbols.allSymbols();
Index.build(std::move(Symbols));
}
+void FileIndex::updateMainFile(PathRef Path, ParsedAST &AST) {
+ SymbolSlab::Builder Builder =
+ indexMainAST(AST.getASTContext(), AST.getPreprocessorPtr(),
+ AST.getLocalTopLevelDecls(), URISchemes);
+ FSymbols.updateMainFile(Path, std::move(Builder));
+ auto Symbols = FSymbols.allSymbols();
+ Index.build(std::move(Symbols));
+}
+
bool FileIndex::fuzzyFind(
const FuzzyFindRequest &Req,
llvm::function_ref<void(const Symbol &)> Callback) const {
Index: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -73,11 +73,12 @@
void onPreambleAST(PathRef Path, ASTContext &Ctx,
std::shared_ptr<clang::Preprocessor> PP) override {
if (FileIdx)
- FileIdx->update(Path, &Ctx, std::move(PP));
+ FileIdx->updatePreamble(Path, &Ctx, std::move(PP));
}
void onMainAST(PathRef Path, ParsedAST &AST) override {
- // FIXME: merge results from the main file into the index too.
+ if (FileIdx)
+ FileIdx->updateMainFile(Path, AST);
}
private:
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
