hokein updated this revision to Diff 143508.
hokein marked 7 inline comments as done.
hokein added a comment.
Refine the patch and address all review comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45717
Files:
clangd/ClangdServer.cpp
clangd/XRefs.cpp
clangd/XRefs.h
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===================================================================
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -13,11 +13,13 @@
#include "SyncAPI.h"
#include "TestFS.h"
#include "XRefs.h"
+#include "gmock/gmock.h"
+#include "index/SymbolCollector.h"
#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Frontend/PCHContainerOperations.h"
#include "clang/Frontend/Utils.h"
+#include "clang/Index/IndexingAction.h"
#include "llvm/Support/Path.h"
-#include "gmock/gmock.h"
#include "gtest/gtest.h"
namespace clang {
@@ -37,17 +39,51 @@
};
// FIXME: this is duplicated with FileIndexTests. Share it.
-ParsedAST build(StringRef Code) {
- auto CI = createInvocationFromCommandLine(
- {"clang", "-xc++", testPath("Foo.cpp").c_str()});
- auto Buf = MemoryBuffer::getMemBuffer(Code);
+ParsedAST build(StringRef MainCode, StringRef HeaderCode = "",
+ StringRef BaseName = "Main",
+ llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem>
+ InMemoryFileSystem = nullptr) {
+ auto HeaderPath = testPath((BaseName + ".h").str());
+ auto MainPath = testPath((BaseName + ".cpp").str());
+ if (!InMemoryFileSystem)
+ InMemoryFileSystem = new vfs::InMemoryFileSystem();
+
+ std::vector<const char *> Cmd = {"clang", "-xc++", MainPath.c_str()};
+ if (!HeaderCode.empty()) {
+ InMemoryFileSystem->addFile(HeaderPath, 0,
+ llvm::MemoryBuffer::getMemBuffer(HeaderCode));
+ std::vector<const char *> args = {"-include", HeaderPath.c_str()};
+ Cmd.insert(Cmd.begin() + 1, args.begin(), args.end());
+ }
+
+ InMemoryFileSystem->addFile(MainPath, 0,
+ llvm::MemoryBuffer::getMemBuffer(MainCode));
+
+ auto CI = createInvocationFromCommandLine(Cmd);
+
+ auto Buf = MemoryBuffer::getMemBuffer(MainCode);
auto AST = ParsedAST::Build(std::move(CI), nullptr, std::move(Buf),
std::make_shared<PCHContainerOperations>(),
- vfs::getRealFileSystem());
+ InMemoryFileSystem);
assert(AST.hasValue());
return std::move(*AST);
}
+std::unique_ptr<SymbolIndex> buildIndexFromAST(ParsedAST *AST) {
+ SymbolCollector::Options CollectorOpts;
+ CollectorOpts.CollectIncludePath = false;
+ CollectorOpts.CountReferences = false;
+ index::IndexingOptions IndexOpts;
+ IndexOpts.SystemSymbolFilter =
+ index::IndexingOptions::SystemSymbolFilterKind::DeclarationsOnly;
+ IndexOpts.IndexFunctionLocals = false;
+ auto Collector = std::make_shared<SymbolCollector>(std::move(CollectorOpts));
+ Collector->setPreprocessor(AST->getPreprocessorPtr());
+ index::indexTopLevelDecls(AST->getASTContext(), AST->getTopLevelDecls(),
+ Collector, IndexOpts);
+ return MemIndex::build(Collector->takeSymbols());
+}
+
// Extracts ranges from an annotated example, and constructs a matcher for a
// highlight set. Ranges should be named $read/$write as appropriate.
Matcher<const std::vector<DocumentHighlight> &>
@@ -106,6 +142,80 @@
MATCHER_P(RangeIs, R, "") { return arg.range == R; }
+TEST(GoToDefinition, WithIndex) {
+ llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem> InMemoryFileSystem(
+ new vfs::InMemoryFileSystem);
+ Annotations SymbolHeader(R"cpp(
+ class $forward[[Forward]];
+ class $foo[[Foo]] {};
+
+ void $f1[[f1]]();
+
+ inline void $f2[[f2]]() {}
+ )cpp");
+ Annotations SymbolCpp(R"cpp(
+ class $forward[[forward]] {};
+ void $f1[[f1]]() {}
+ )cpp");
+
+ // Build index.
+ auto IndexAST = build(SymbolCpp.code(), SymbolHeader.code(), "symbol",
+ InMemoryFileSystem);
+ auto Index = buildIndexFromAST(&IndexAST);
+
+ auto runFindDefinitionsWithIndex =
+ [&Index, &InMemoryFileSystem](const Annotations &Main) {
+ auto AST = build(/*MainCode=*/Main.code(),
+ /*HeaderCode=*/"",
+ /*BasePath=*/"main", InMemoryFileSystem);
+ return clangd::findDefinitions(AST, Main.point(), Index.get());
+ };
+
+ Annotations Test(R"cpp(// only declaration in AST.
+ void [[f1]]();
+ int main() {
+ ^f1();
+ }
+ )cpp");
+ EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+ testing::ElementsAreArray(
+ {RangeIs(SymbolCpp.range("f1")), RangeIs(Test.range())}));
+
+ Test = Annotations(R"cpp(// definition in AST.
+ void [[f1]]() {}
+ int main() {
+ ^f1();
+ }
+ )cpp");
+ EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+ testing::ElementsAreArray(
+ {RangeIs(Test.range()), RangeIs(SymbolHeader.range("f1"))}));
+
+ Test = Annotations(R"cpp(// forward declaration in AST.
+ class [[Foo]];
+ F^oo* create();
+ )cpp");
+ EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+ testing::ElementsAreArray(
+ {RangeIs(SymbolHeader.range("foo")), RangeIs(Test.range())}));
+
+ Test = Annotations(R"cpp(// defintion in AST.
+ class [[Forward]] {};
+ F^orward create();
+ )cpp");
+ EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+ testing::ElementsAreArray({
+ RangeIs(Test.range()), RangeIs(SymbolHeader.range("forward")),
+ }));
+
+ Test = Annotations(R"cpp(// forward declaration in AST.
+ #include "symbol.h"
+ F^oo* create();
+ )cpp");
+ EXPECT_THAT(runFindDefinitionsWithIndex(Test),
+ testing::ElementsAreArray({RangeIs(SymbolHeader.range("foo"))}));
+}
+
TEST(GoToDefinition, All) {
const char *Tests[] = {
R"cpp(// Local variable
Index: clangd/XRefs.h
===================================================================
--- clangd/XRefs.h
+++ clangd/XRefs.h
@@ -15,13 +15,15 @@
#include "ClangdUnit.h"
#include "Protocol.h"
+#include "index/Index.h"
#include <vector>
namespace clang {
namespace clangd {
/// Get definition of symbol at a specified \p Pos.
-std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos);
+std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
+ const SymbolIndex *Index = nullptr);
/// Returns highlights for all usages of a symbol at \p Pos.
std::vector<DocumentHighlight> findDocumentHighlights(ParsedAST &AST,
Index: clangd/XRefs.cpp
===================================================================
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -14,6 +14,7 @@
#include "clang/AST/DeclTemplate.h"
#include "clang/Index/IndexDataConsumer.h"
#include "clang/Index/IndexingAction.h"
+#include "clang/Index/USRGeneration.h"
#include "llvm/Support/Path.h"
namespace clang {
namespace clangd {
@@ -128,6 +129,38 @@
}
};
+struct IdentifiedSymbol {
+ std::vector<const Decl *> Decls;
+ std::vector<MacroDecl> Macros;
+};
+
+IdentifiedSymbol getSymbolAtPosition(ParsedAST &AST, SourceLocation Pos) {
+ auto DeclMacrosFinder = std::make_shared<DeclarationAndMacrosFinder>(
+ llvm::errs(), Pos, AST.getASTContext(), AST.getPreprocessor());
+ index::IndexingOptions IndexOpts;
+ IndexOpts.SystemSymbolFilter =
+ index::IndexingOptions::SystemSymbolFilterKind::All;
+ IndexOpts.IndexFunctionLocals = true;
+ indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
+ DeclMacrosFinder, IndexOpts);
+
+ return {DeclMacrosFinder->takeDecls(), DeclMacrosFinder->takeMacroInfos()};
+}
+
+llvm::Optional<std::string>
+getAbsoluteFilePath(const FileEntry *F, const SourceManager &SourceMgr) {
+ SmallString<64> FilePath = F->tryGetRealPathName();
+ if (FilePath.empty())
+ FilePath = F->getName();
+ if (!llvm::sys::path::is_absolute(FilePath)) {
+ if (!SourceMgr.getFileManager().makeAbsolutePath(FilePath)) {
+ log("Could not turn relative path to absolute: " + FilePath);
+ return llvm::None;
+ }
+ }
+ return FilePath.str().str();
+}
+
llvm::Optional<Location>
makeLocation(ParsedAST &AST, const SourceRange &ValSourceRange) {
const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
@@ -145,24 +178,18 @@
Range R = {Begin, End};
Location L;
- SmallString<64> FilePath = F->tryGetRealPathName();
- if (FilePath.empty())
- FilePath = F->getName();
- if (!llvm::sys::path::is_absolute(FilePath)) {
- if (!SourceMgr.getFileManager().makeAbsolutePath(FilePath)) {
- log("Could not turn relative path to absolute: " + FilePath);
- return llvm::None;
- }
- }
-
- L.uri = URIForFile(FilePath.str());
+ auto FilePath = getAbsoluteFilePath(F, SourceMgr);
+ if (!FilePath)
+ return llvm::None;
+ L.uri = URIForFile(*FilePath);
L.range = R;
return L;
}
} // namespace
-std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos) {
+std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
+ const SymbolIndex *Index) {
const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
const FileEntry *FE = SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
if (!FE)
@@ -182,32 +209,108 @@
if (!Result.empty())
return Result;
- auto DeclMacrosFinder = std::make_shared<DeclarationAndMacrosFinder>(
- llvm::errs(), SourceLocationBeg, AST.getASTContext(),
- AST.getPreprocessor());
- index::IndexingOptions IndexOpts;
- IndexOpts.SystemSymbolFilter =
- index::IndexingOptions::SystemSymbolFilterKind::All;
- IndexOpts.IndexFunctionLocals = true;
+ // Identified symbols at a specific position.
+ auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
- indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
- DeclMacrosFinder, IndexOpts);
+ // Handle goto definition for macros.
+ if (!Symbols.Macros.empty()) {
+ for (auto Item : Symbols.Macros) {
+ auto Loc = Item.Info->getDefinitionLoc();
+ auto L = makeLocation(AST, SourceRange(Loc, Loc));
+ if (L)
+ Result.push_back(*L);
+ }
+ return Result;
+ }
- std::vector<const Decl *> Decls = DeclMacrosFinder->takeDecls();
- std::vector<MacroDecl> MacroInfos = DeclMacrosFinder->takeMacroInfos();
+ // Handle goto definition for typical declarations.
+ //
+ // Declaration and definition are different terms in C-family languages, and
+ // LSP only defines the "GoToDefinition" specification, so we try to perform
+ // the "most sensible" GoTo operation:
+ //
+ // - We use the location from AST and index (if available) to provide the
+ // final results. Prefer AST over index.
+ //
+ // - For each symbol, we will return a location of the cannonical declaration
+ // (e.g. function declaration in header), and a location of definition if
+ // they are available, definition comes first.
+ struct CandidateLocation {
+ llvm::Optional<Location> Def;
+ llvm::Optional<Location> Decl;
+ };
+ llvm::DenseMap<SymbolID, CandidateLocation> ResultCandidates;
+
+ LookupRequest QueryRequest;
+
+ for (auto D : Symbols.Decls) {
+ llvm::SmallString<128> USR;
+ if (index::generateUSRForDecl(D, USR))
+ continue;
- for (auto D : Decls) {
auto Loc = findNameLoc(D);
auto L = makeLocation(AST, SourceRange(Loc, Loc));
- if (L)
- Result.push_back(*L);
+ auto ID = SymbolID(USR);
+ QueryRequest.IDs.insert(ID);
+ CandidateLocation &Candidate = ResultCandidates[ID];
+ bool IsDef = GetDefinition(D) == D;
+ // Populate one of the slots with location for the AST.
+ if (!IsDef)
+ Candidate.Decl = L;
+ else
+ Candidate.Def = L;
}
- for (auto Item : MacroInfos) {
- auto Loc = Item.Info->getDefinitionLoc();
- auto L = makeLocation(AST, SourceRange(Loc, Loc));
- if (L)
- Result.push_back(*L);
+ if (Index) {
+ log("query index...");
+ std::string HintPath;
+ if (auto Path = getAbsoluteFilePath(FE, SourceMgr))
+ HintPath = *Path;
+ // Query the index and populate the empty slot.
+ Index->lookup(QueryRequest,
+ [&HintPath, &ResultCandidates](const Symbol &Sym) {
+ auto It = ResultCandidates.find(Sym.ID);
+ assert(It != ResultCandidates.end());
+
+ // FIXME: duplicate function with workspace symbols, share
+ // it.
+ auto ToLSPLocation = [&HintPath](
+ const SymbolLocation &Loc) -> llvm::Optional<Location> {
+ if (!Loc)
+ return llvm::None;
+ auto Uri = URI::parse(Loc.FileURI);
+ if (!Uri) {
+ log("Could not parse URI: " + Loc.FileURI);
+ return llvm::None;
+ }
+ auto Path = URI::resolve(*Uri, HintPath);
+ if (!Path) {
+ log("Could not resolve URI");
+ return llvm::None;
+ }
+ Location LSPLoc;
+ LSPLoc.uri = URIForFile(*Path);
+ LSPLoc.range.start.line = Loc.Start.Line;
+ LSPLoc.range.start.character = Loc.Start.Column;
+ LSPLoc.range.end.line = Loc.End.Line;
+ LSPLoc.range.end.character = Loc.End.Column;
+ return LSPLoc;
+ };
+
+ if (!It->second.Def)
+ It->second.Def = ToLSPLocation(Sym.Definition);
+ if (!It->second.Decl)
+ It->second.Decl = ToLSPLocation(Sym.CanonicalDeclaration);
+ });
+ }
+
+ // Populate the results, definition first.
+ for (auto It : ResultCandidates) {
+ auto Loc = It.second;
+ if (Loc.Def)
+ Result.push_back(*Loc.Def);
+ if (Loc.Decl && Loc.Decl != Loc.Def) // Decl and Def might be the same
+ Result.push_back(*(Loc.Decl));
}
return Result;
@@ -286,23 +389,16 @@
SourceLocation SourceLocationBeg = getBeginningOfIdentifier(AST, Pos, FE);
- auto DeclMacrosFinder = std::make_shared<DeclarationAndMacrosFinder>(
- llvm::errs(), SourceLocationBeg, AST.getASTContext(),
- AST.getPreprocessor());
- index::IndexingOptions IndexOpts;
- IndexOpts.SystemSymbolFilter =
- index::IndexingOptions::SystemSymbolFilterKind::All;
- IndexOpts.IndexFunctionLocals = true;
-
- // Macro occurences are not currently handled.
- indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
- DeclMacrosFinder, IndexOpts);
-
- std::vector<const Decl *> SelectedDecls = DeclMacrosFinder->takeDecls();
+ auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
+ std::vector<const Decl *> SelectedDecls = Symbols.Decls;
auto DocHighlightsFinder = std::make_shared<DocumentHighlightsFinder>(
llvm::errs(), AST.getASTContext(), AST.getPreprocessor(), SelectedDecls);
+ index::IndexingOptions IndexOpts;
+ IndexOpts.SystemSymbolFilter =
+ index::IndexingOptions::SystemSymbolFilterKind::All;
+ IndexOpts.IndexFunctionLocals = true;
indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
DocHighlightsFinder, IndexOpts);
Index: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -360,11 +360,11 @@
void ClangdServer::findDefinitions(PathRef File, Position Pos,
Callback<std::vector<Location>> CB) {
auto FS = FSProvider.getFileSystem();
- auto Action = [Pos, FS](Callback<std::vector<Location>> CB,
- llvm::Expected<InputsAndAST> InpAST) {
+ auto Action = [Pos, FS, this](Callback<std::vector<Location>> CB,
+ llvm::Expected<InputsAndAST> InpAST) {
if (!InpAST)
return CB(InpAST.takeError());
- CB(clangd::findDefinitions(InpAST->AST, Pos));
+ CB(clangd::findDefinitions(InpAST->AST, Pos, this->FileIdx.get()));
};
WorkScheduler.runWithAST("Definitions", File, Bind(Action, std::move(CB)));
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
