[PATCH] D47846: [clangd] Implementation of textDocument/documentSymbol
This revision was automatically updated to reflect the committed changes.
Closed by commit rL336386: [clangd] Implementation of
textDocument/documentSymbol (authored by malaperle, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D47846
Files:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/ClangdLSPServer.h
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/FindSymbols.cpp
clang-tools-extra/trunk/clangd/FindSymbols.h
clang-tools-extra/trunk/clangd/Protocol.cpp
clang-tools-extra/trunk/clangd/Protocol.h
clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
clang-tools-extra/trunk/clangd/ProtocolHandlers.h
clang-tools-extra/trunk/clangd/SourceCode.cpp
clang-tools-extra/trunk/clangd/SourceCode.h
clang-tools-extra/trunk/clangd/XRefs.cpp
clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test
clang-tools-extra/trunk/test/clangd/initialize-params.test
clang-tools-extra/trunk/test/clangd/symbols.test
clang-tools-extra/trunk/unittests/clangd/FindSymbolsTests.cpp
clang-tools-extra/trunk/unittests/clangd/SyncAPI.cpp
clang-tools-extra/trunk/unittests/clangd/SyncAPI.h
Index: clang-tools-extra/trunk/unittests/clangd/SyncAPI.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/SyncAPI.cpp
+++ clang-tools-extra/trunk/unittests/clangd/SyncAPI.cpp
@@ -117,5 +117,12 @@
return std::move(*Result);
}
+llvm::Expected>
+runDocumentSymbols(ClangdServer , PathRef File) {
+ llvm::Optional>> Result;
+ Server.documentSymbols(File, capture(Result));
+ return std::move(*Result);
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/trunk/unittests/clangd/FindSymbolsTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/FindSymbolsTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/FindSymbolsTests.cpp
@@ -22,6 +22,7 @@
using ::testing::AllOf;
using ::testing::AnyOf;
using ::testing::ElementsAre;
+using ::testing::ElementsAreArray;
using ::testing::IsEmpty;
using ::testing::UnorderedElementsAre;
@@ -37,6 +38,7 @@
return (arg.containerName + "::" + arg.name) == Name;
}
MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SymRange, Range, "") { return arg.location.range == Range; }
ClangdServer::Options optsForTests() {
auto ServerOpts = ClangdServer::optsForTest();
@@ -287,5 +289,274 @@
EXPECT_THAT(getSymbols("foo"), ElementsAre(QName("foo")));
}
+namespace {
+class DocumentSymbolsTest : public ::testing::Test {
+public:
+ DocumentSymbolsTest()
+ : Server(CDB, FSProvider, DiagConsumer, optsForTests()) {}
+
+protected:
+ MockFSProvider FSProvider;
+ MockCompilationDatabase CDB;
+ IgnoreDiagnostics DiagConsumer;
+ ClangdServer Server;
+
+ std::vector getSymbols(PathRef File) {
+EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for preamble";
+auto SymbolInfos = runDocumentSymbols(Server, File);
+EXPECT_TRUE(bool(SymbolInfos)) << "documentSymbols returned an error";
+return *SymbolInfos;
+ }
+
+ void addFile(StringRef FilePath, StringRef Contents) {
+FSProvider.Files[FilePath] = Contents;
+Server.addDocument(FilePath, Contents);
+ }
+};
+} // namespace
+
+TEST_F(DocumentSymbolsTest, BasicSymbols) {
+ std::string FilePath = testPath("foo.cpp");
+ Annotations Main(R"(
+ class Foo;
+ class Foo {
+Foo() {}
+Foo(int a) {}
+void $decl[[f]]();
+friend void f1();
+friend class Friend;
+Foo& operator=(const Foo&);
+~Foo();
+class Nested {
+void f();
+};
+ };
+ class Friend {
+ };
+
+ void f1();
+ inline void f2() {}
+ static const int KInt = 2;
+ const char* kStr = "123";
+
+ void f1() {}
+
+ namespace foo {
+ // Type alias
+ typedef int int32;
+ using int32_t = int32;
+
+ // Variable
+ int v1;
+
+ // Namespace
+ namespace bar {
+ int v2;
+ }
+ // Namespace alias
+ namespace baz = bar;
+
+ // FIXME: using declaration is not supported as the IndexAction will ignore
+ // implicit declarations (the implicit using shadow declaration) by default,
+ // and there is no way to customize this behavior at the moment.
+ using bar::v2;
+ } // namespace foo
+)");
+
+ addFile(FilePath, Main.code());
+ EXPECT_THAT(getSymbols(FilePath),
+ ElementsAreArray(
+ {AllOf(QName("Foo"), WithKind(SymbolKind::Class)),
+ AllOf(QName("Foo"), WithKind(SymbolKind::Class)),
+ AllOf(QName("Foo::Foo"), WithKind(SymbolKind::Method)),
+ AllOf(QName("Foo::Foo"), WithKind(SymbolKind::Method)),
+
[PATCH] D47846: [clangd] Implementation of textDocument/documentSymbol
malaperle added a comment. Thanks a lot for the great comments (as always)! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D47846 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47846: [clangd] Implementation of textDocument/documentSymbol
malaperle updated this revision to Diff 154280.
malaperle marked 7 inline comments as done.
malaperle added a comment.
Address comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47846
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/FindSymbols.cpp
clangd/FindSymbols.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/XRefs.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
test/clangd/symbols.test
unittests/clangd/FindSymbolsTests.cpp
unittests/clangd/SyncAPI.cpp
unittests/clangd/SyncAPI.h
Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -43,6 +43,9 @@
llvm::Expected>
runWorkspaceSymbols(ClangdServer , StringRef Query, int Limit);
+llvm::Expected>
+runDocumentSymbols(ClangdServer , PathRef File);
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -117,5 +117,12 @@
return std::move(*Result);
}
+llvm::Expected>
+runDocumentSymbols(ClangdServer , PathRef File) {
+ llvm::Optional>> Result;
+ Server.documentSymbols(File, capture(Result));
+ return std::move(*Result);
+}
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- unittests/clangd/FindSymbolsTests.cpp
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -22,6 +22,7 @@
using ::testing::AllOf;
using ::testing::AnyOf;
using ::testing::ElementsAre;
+using ::testing::ElementsAreArray;
using ::testing::IsEmpty;
using ::testing::UnorderedElementsAre;
@@ -37,6 +38,7 @@
return (arg.containerName + "::" + arg.name) == Name;
}
MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SymRange, Range, "") { return arg.location.range == Range; }
ClangdServer::Options optsForTests() {
auto ServerOpts = ClangdServer::optsForTest();
@@ -287,5 +289,274 @@
EXPECT_THAT(getSymbols("foo"), ElementsAre(QName("foo")));
}
+namespace {
+class DocumentSymbolsTest : public ::testing::Test {
+public:
+ DocumentSymbolsTest()
+ : Server(CDB, FSProvider, DiagConsumer, optsForTests()) {}
+
+protected:
+ MockFSProvider FSProvider;
+ MockCompilationDatabase CDB;
+ IgnoreDiagnostics DiagConsumer;
+ ClangdServer Server;
+
+ std::vector getSymbols(PathRef File) {
+EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for preamble";
+auto SymbolInfos = runDocumentSymbols(Server, File);
+EXPECT_TRUE(bool(SymbolInfos)) << "documentSymbols returned an error";
+return *SymbolInfos;
+ }
+
+ void addFile(StringRef FilePath, StringRef Contents) {
+FSProvider.Files[FilePath] = Contents;
+Server.addDocument(FilePath, Contents);
+ }
+};
+} // namespace
+
+TEST_F(DocumentSymbolsTest, BasicSymbols) {
+ std::string FilePath = testPath("foo.cpp");
+ Annotations Main(R"(
+ class Foo;
+ class Foo {
+Foo() {}
+Foo(int a) {}
+void $decl[[f]]();
+friend void f1();
+friend class Friend;
+Foo& operator=(const Foo&);
+~Foo();
+class Nested {
+void f();
+};
+ };
+ class Friend {
+ };
+
+ void f1();
+ inline void f2() {}
+ static const int KInt = 2;
+ const char* kStr = "123";
+
+ void f1() {}
+
+ namespace foo {
+ // Type alias
+ typedef int int32;
+ using int32_t = int32;
+
+ // Variable
+ int v1;
+
+ // Namespace
+ namespace bar {
+ int v2;
+ }
+ // Namespace alias
+ namespace baz = bar;
+
+ // FIXME: using declaration is not supported as the IndexAction will ignore
+ // implicit declarations (the implicit using shadow declaration) by default,
+ // and there is no way to customize this behavior at the moment.
+ using bar::v2;
+ } // namespace foo
+)");
+
+ addFile(FilePath, Main.code());
+ EXPECT_THAT(getSymbols(FilePath),
+ ElementsAreArray(
+ {AllOf(QName("Foo"), WithKind(SymbolKind::Class)),
+ AllOf(QName("Foo"), WithKind(SymbolKind::Class)),
+ AllOf(QName("Foo::Foo"), WithKind(SymbolKind::Method)),
+ AllOf(QName("Foo::Foo"), WithKind(SymbolKind::Method)),
+ AllOf(QName("Foo::f"), WithKind(SymbolKind::Method)),
+ AllOf(QName("f1"), WithKind(SymbolKind::Function)),
+ AllOf(QName("Foo::operator="), WithKind(SymbolKind::Method)),
+ AllOf(QName("Foo::~Foo"), WithKind(SymbolKind::Method)),
+
[PATCH] D47846: [clangd] Implementation of textDocument/documentSymbol
sammccall added a comment. (Reasoning for not using SymbolCollector totally makes sense, thanks for the breakdown!) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D47846 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47846: [clangd] Implementation of textDocument/documentSymbol
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Sorry, I thought I'd accepted this already!
Comment at: clangd/ClangdServer.cpp:467
+ };
+ WorkScheduler.runWithAST("Hover", File, Bind(Action, std::move(CB)));
+}
hover -> documentSymbols
Comment at: clangd/FindSymbols.cpp:188
+public:
+ DocumentSymbolsConsumer(raw_ostream , ASTContext ) : AST(AST) {}
+ std::vector takeSymbols() { return std::move(Symbols); }
OS is unused
Comment at: clangd/FindSymbols.cpp:203
+
+ bool shouldFilterDecl(const NamedDecl *ND) {
+if (!ND || ND->isImplicit())
nit: rename to shouldIncludeSymbol or something? "filter" is slightly
ambiguous, and a negation. (We've made this change in SymbolCollector recently)
Comment at: clangd/FindSymbols.cpp:229
+// We should be only be looking at "local" decls in the main file.
+if (SourceMgr.getMainFileID() != SourceMgr.getFileID(NameLoc)) {
+ // Even thought we are visiting only local (non-preamble) decls,
nit: isWrittenInMainFile(NameLoc)
Comment at: clangd/FindSymbols.cpp:234
+}
+// assert(SourceMgr.getMainFileID() == SourceMgr.getFileID(NameLoc));
+const NamedDecl *ND = llvm::dyn_cast(ASTNode.OrigD);
remove
Comment at: clangd/FindSymbols.cpp:256
+
+Symbols.push_back({Name, SK, L, Scope});
+return true;
nit: for readability, could we declare a local SymbolInformation above, fill in
the fields by name, and then move it into Symbols?
a) it's easier to see that the right values go into the right slots, b), it's
easier to search (find references!) for how fields are populated.
Comment at: clangd/FindSymbols.cpp:268
+ index::IndexingOptions IndexOpts;
+ // We don't need references, only declarations so that is makes a nice
+ // outline of the document.
applying the filter is fine, but note that SystemSymbolFilter only applies to
included symbols in -isystem headers (I only just noticed that myself). So the
comment is a little misleading, I would drop it.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47846
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47846: [clangd] Implementation of textDocument/documentSymbol
malaperle updated this revision to Diff 153742.
malaperle added a comment.
Rebased.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47846
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/FindSymbols.cpp
clangd/FindSymbols.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/XRefs.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
test/clangd/symbols.test
unittests/clangd/FindSymbolsTests.cpp
unittests/clangd/SyncAPI.cpp
unittests/clangd/SyncAPI.h
Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -43,6 +43,9 @@
llvm::Expected>
runWorkspaceSymbols(ClangdServer , StringRef Query, int Limit);
+llvm::Expected>
+runDocumentSymbols(ClangdServer , PathRef File);
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -117,5 +117,12 @@
return std::move(*Result);
}
+llvm::Expected>
+runDocumentSymbols(ClangdServer , PathRef File) {
+ llvm::Optional>> Result;
+ Server.documentSymbols(File, capture(Result));
+ return std::move(*Result);
+}
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- unittests/clangd/FindSymbolsTests.cpp
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -22,6 +22,7 @@
using ::testing::AllOf;
using ::testing::AnyOf;
using ::testing::ElementsAre;
+using ::testing::ElementsAreArray;
using ::testing::IsEmpty;
using ::testing::UnorderedElementsAre;
@@ -37,6 +38,7 @@
return (arg.containerName + "::" + arg.name) == Name;
}
MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SymRange, Range, "") { return arg.location.range == Range; }
ClangdServer::Options optsForTests() {
auto ServerOpts = ClangdServer::optsForTest();
@@ -287,5 +289,274 @@
EXPECT_THAT(getSymbols("foo"), ElementsAre(QName("foo")));
}
+namespace {
+class DocumentSymbolsTest : public ::testing::Test {
+public:
+ DocumentSymbolsTest()
+ : Server(CDB, FSProvider, DiagConsumer, optsForTests()) {}
+
+protected:
+ MockFSProvider FSProvider;
+ MockCompilationDatabase CDB;
+ IgnoreDiagnostics DiagConsumer;
+ ClangdServer Server;
+
+ std::vector getSymbols(PathRef File) {
+EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for preamble";
+auto SymbolInfos = runDocumentSymbols(Server, File);
+EXPECT_TRUE(bool(SymbolInfos)) << "documentSymbols returned an error";
+return *SymbolInfos;
+ }
+
+ void addFile(StringRef FilePath, StringRef Contents) {
+FSProvider.Files[FilePath] = Contents;
+Server.addDocument(FilePath, Contents);
+ }
+};
+} // namespace
+
+TEST_F(DocumentSymbolsTest, BasicSymbols) {
+ std::string FilePath = testPath("foo.cpp");
+ Annotations Main(R"(
+ class Foo;
+ class Foo {
+Foo() {}
+Foo(int a) {}
+void $decl[[f]]();
+friend void f1();
+friend class Friend;
+Foo& operator=(const Foo&);
+~Foo();
+class Nested {
+void f();
+};
+ };
+ class Friend {
+ };
+
+ void f1();
+ inline void f2() {}
+ static const int KInt = 2;
+ const char* kStr = "123";
+
+ void f1() {}
+
+ namespace foo {
+ // Type alias
+ typedef int int32;
+ using int32_t = int32;
+
+ // Variable
+ int v1;
+
+ // Namespace
+ namespace bar {
+ int v2;
+ }
+ // Namespace alias
+ namespace baz = bar;
+
+ // FIXME: using declaration is not supported as the IndexAction will ignore
+ // implicit declarations (the implicit using shadow declaration) by default,
+ // and there is no way to customize this behavior at the moment.
+ using bar::v2;
+ } // namespace foo
+)");
+
+ addFile(FilePath, Main.code());
+ EXPECT_THAT(getSymbols(FilePath),
+ ElementsAreArray(
+ {AllOf(QName("Foo"), WithKind(SymbolKind::Class)),
+ AllOf(QName("Foo"), WithKind(SymbolKind::Class)),
+ AllOf(QName("Foo::Foo"), WithKind(SymbolKind::Method)),
+ AllOf(QName("Foo::Foo"), WithKind(SymbolKind::Method)),
+ AllOf(QName("Foo::f"), WithKind(SymbolKind::Method)),
+ AllOf(QName("f1"), WithKind(SymbolKind::Function)),
+ AllOf(QName("Foo::operator="), WithKind(SymbolKind::Method)),
+ AllOf(QName("Foo::~Foo"), WithKind(SymbolKind::Method)),
+ AllOf(QName("Foo::Nested"), WithKind(SymbolKind::Class)),
+
[PATCH] D47846: [clangd] Implementation of textDocument/documentSymbol
malaperle updated this revision to Diff 153611.
malaperle added a comment.
Fix handling of externs, definition vs declaration and call more common code.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47846
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/FindSymbols.cpp
clangd/FindSymbols.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/XRefs.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
test/clangd/symbols.test
unittests/clangd/FindSymbolsTests.cpp
unittests/clangd/SyncAPI.cpp
unittests/clangd/SyncAPI.h
Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -43,6 +43,9 @@
llvm::Expected>
runWorkspaceSymbols(ClangdServer , StringRef Query, int Limit);
+llvm::Expected>
+runDocumentSymbols(ClangdServer , PathRef File);
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -117,5 +117,12 @@
return std::move(*Result);
}
+llvm::Expected>
+runDocumentSymbols(ClangdServer , PathRef File) {
+ llvm::Optional>> Result;
+ Server.documentSymbols(File, capture(Result));
+ return std::move(*Result);
+}
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- unittests/clangd/FindSymbolsTests.cpp
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -22,6 +22,7 @@
using ::testing::AllOf;
using ::testing::AnyOf;
using ::testing::ElementsAre;
+using ::testing::ElementsAreArray;
using ::testing::IsEmpty;
using ::testing::UnorderedElementsAre;
@@ -37,6 +38,7 @@
return (arg.containerName + "::" + arg.name) == Name;
}
MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SymRange, Range, "") { return arg.location.range == Range; }
ClangdServer::Options optsForTests() {
auto ServerOpts = ClangdServer::optsForTest();
@@ -287,5 +289,274 @@
EXPECT_THAT(getSymbols("foo"), ElementsAre(QName("foo")));
}
+namespace {
+class DocumentSymbolsTest : public ::testing::Test {
+public:
+ DocumentSymbolsTest()
+ : Server(CDB, FSProvider, DiagConsumer, optsForTests()) {}
+
+protected:
+ MockFSProvider FSProvider;
+ MockCompilationDatabase CDB;
+ IgnoreDiagnostics DiagConsumer;
+ ClangdServer Server;
+
+ std::vector getSymbols(PathRef File) {
+EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for preamble";
+auto SymbolInfos = runDocumentSymbols(Server, File);
+EXPECT_TRUE(bool(SymbolInfos)) << "documentSymbols returned an error";
+return *SymbolInfos;
+ }
+
+ void addFile(StringRef FilePath, StringRef Contents) {
+FSProvider.Files[FilePath] = Contents;
+Server.addDocument(FilePath, Contents);
+ }
+};
+} // namespace
+
+TEST_F(DocumentSymbolsTest, BasicSymbols) {
+ std::string FilePath = testPath("foo.cpp");
+ Annotations Main(R"(
+ class Foo;
+ class Foo {
+Foo() {}
+Foo(int a) {}
+void $decl[[f]]();
+friend void f1();
+friend class Friend;
+Foo& operator=(const Foo&);
+~Foo();
+class Nested {
+void f();
+};
+ };
+ class Friend {
+ };
+
+ void f1();
+ inline void f2() {}
+ static const int KInt = 2;
+ const char* kStr = "123";
+
+ void f1() {}
+
+ namespace foo {
+ // Type alias
+ typedef int int32;
+ using int32_t = int32;
+
+ // Variable
+ int v1;
+
+ // Namespace
+ namespace bar {
+ int v2;
+ }
+ // Namespace alias
+ namespace baz = bar;
+
+ // FIXME: using declaration is not supported as the IndexAction will ignore
+ // implicit declarations (the implicit using shadow declaration) by default,
+ // and there is no way to customize this behavior at the moment.
+ using bar::v2;
+ } // namespace foo
+)");
+
+ addFile(FilePath, Main.code());
+ EXPECT_THAT(getSymbols(FilePath),
+ ElementsAreArray(
+ {AllOf(QName("Foo"), WithKind(SymbolKind::Class)),
+ AllOf(QName("Foo"), WithKind(SymbolKind::Class)),
+ AllOf(QName("Foo::Foo"), WithKind(SymbolKind::Method)),
+ AllOf(QName("Foo::Foo"), WithKind(SymbolKind::Method)),
+ AllOf(QName("Foo::f"), WithKind(SymbolKind::Method)),
+ AllOf(QName("f1"), WithKind(SymbolKind::Function)),
+ AllOf(QName("Foo::operator="), WithKind(SymbolKind::Method)),
+ AllOf(QName("Foo::~Foo"), WithKind(SymbolKind::Method)),
+
[PATCH] D47846: [clangd] Implementation of textDocument/documentSymbol
malaperle added inline comments.
Comment at: clangd/FindSymbols.cpp:181
+/// Finds document symbols in the main file of the AST.
+class DocumentSymbolsConsumer : public index::IndexDataConsumer {
+ ASTContext
sammccall wrote:
> I guess the alternative here is to use SymbolCollector and then convert
> Symbol->SymbolInformation (which we should already have for workspace/symbol).
>
> I also guess there's some divergence in behavior, which is why you went this
> route :-)
> Mostly in filtering? (I'd think that for e.g. printing, we'd want to be
> consistent)
>
> Do you think it's worth adding enough customization to SymbolCollector to
> make it usable here? Even if it was making `shouldFilterDecl` a
> std::function, there'd be some value in unifying the rest. WDYT?
I put a bit of the justification in the summary, perhaps you missed it or maybe
did you think it was not enough of a justification?
> An AST-based approach is used to retrieve the document symbols rather than an
> in-memory index query. The index is not an ideal fit to achieve this because
> of
> the file-centric query being done here whereas the index is suited for
> project-wide queries. Document symbols also includes more symbols and need to
> keep the order as seen in the file.
It's not a clear cut decision but I felt that there was more diverging parts
than common and that it would be detrimental to SymbolCollector in terms of
added complexity.
Differences:
- We need a different way to filter (as you suggested)
- Don't need anything about completion
- Don't need to distinguish declaration vs definition or canonical declaration
- Don't need a map, we just need them in same order as they appear
and SymbolCollector should be free to store them in whichever order for
purposes of proving a project-wide collection of symbols
- Don't want to track references
- DocumentSymbols needs symbols in main files
Common things:
- Both need to generate Position/Uri from a SourceLocation. But they can both
call sourceLocToPosition.
- Both need to generate a symbol name from Decl&. But they can both call
AST.h's printQualifiedName (I fixed this in a new version).
- Both use/extend a IndexDataConsumer. Not worth sharing the same code path
just to not have to create a class definition IMO.
Let me know if that makes sense to you, otherwise I can try to make another
version that uses SymbolCollector.
Comment at: unittests/clangd/FindSymbolsTests.cpp:447
+ EXPECT_THAT(getSymbols(FilePath),
+ ElementsAre(AllOf(QName("Tmpl"), WithKind(SymbolKind::Struct)),
+ AllOf(QName("Tmpl::x"), WithKind(SymbolKind::Field)),
sammccall wrote:
> in a perfect world, I think the template definitions might be printed
> `Tmpl`, `Tmpl`. Not sure about `Tmpl::x` vs `Tmpl::x` though. WDYT?
>
> (Not necessarily worth doing this patch, keep it simple)
I'm thinking `Tmpl`, `Tmpl`, so that we can more easily
distinguish between a primary and specialization, especially in partial
specializations and when the generic type might not be just a single letter.
And `Tmpl::x`. Maybe this is too much :)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47846
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47846: [clangd] Implementation of textDocument/documentSymbol
sammccall added inline comments.
Comment at: clangd/SourceCode.cpp:194
+ if (!llvm::sys::path::is_absolute(FilePath)) {
+if (!SourceMgr.getFileManager().makeAbsolutePath(FilePath)) {
+ log("Could not turn relative path to absolute: " + FilePath);
sammccall wrote:
> the common case when tryGetRealPathName() is empty seems to be when it's a
> file that was part of the preamble we're reusing.
> Does this fallback tend to give the same answer in that case? (If so, great!
> I know some other places we should reuse this function!)
Sorry, I just noticed this is only being moved. Please disregard (and thangs
for moving it somewhere common).
Note that D48687 modifies this function, let's make sure not to lose anything
in the merge.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47846
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47846: [clangd] Implementation of textDocument/documentSymbol
sammccall added a comment.
This looks great! Would like to get your thoughts on reusing/not reusing
SymbolCollector.
Comment at: clangd/FindSymbols.cpp:181
+/// Finds document symbols in the main file of the AST.
+class DocumentSymbolsConsumer : public index::IndexDataConsumer {
+ ASTContext
I guess the alternative here is to use SymbolCollector and then convert
Symbol->SymbolInformation (which we should already have for workspace/symbol).
I also guess there's some divergence in behavior, which is why you went this
route :-)
Mostly in filtering? (I'd think that for e.g. printing, we'd want to be
consistent)
Do you think it's worth adding enough customization to SymbolCollector to make
it usable here? Even if it was making `shouldFilterDecl` a std::function,
there'd be some value in unifying the rest. WDYT?
Comment at: clangd/SourceCode.cpp:194
+ if (!llvm::sys::path::is_absolute(FilePath)) {
+if (!SourceMgr.getFileManager().makeAbsolutePath(FilePath)) {
+ log("Could not turn relative path to absolute: " + FilePath);
the common case when tryGetRealPathName() is empty seems to be when it's a file
that was part of the preamble we're reusing.
Does this fallback tend to give the same answer in that case? (If so, great! I
know some other places we should reuse this function!)
Comment at: unittests/clangd/FindSymbolsTests.cpp:447
+ EXPECT_THAT(getSymbols(FilePath),
+ ElementsAre(AllOf(QName("Tmpl"), WithKind(SymbolKind::Struct)),
+ AllOf(QName("Tmpl::x"), WithKind(SymbolKind::Field)),
in a perfect world, I think the template definitions might be printed
`Tmpl`, `Tmpl`. Not sure about `Tmpl::x` vs `Tmpl::x` though. WDYT?
(Not necessarily worth doing this patch, keep it simple)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47846
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47846: [clangd] Implementation of textDocument/documentSymbol
malaperle planned changes to this revision. malaperle added a comment. I found some issues while testing, I will investigate before review. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D47846 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47846: [clangd] Implementation of textDocument/documentSymbol
malaperle updated this revision to Diff 152953.
malaperle added a comment.
Rebased.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47846
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/FindSymbols.cpp
clangd/FindSymbols.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/XRefs.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
test/clangd/symbols.test
unittests/clangd/FindSymbolsTests.cpp
unittests/clangd/SyncAPI.cpp
unittests/clangd/SyncAPI.h
Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -43,6 +43,9 @@
llvm::Expected>
runWorkspaceSymbols(ClangdServer , StringRef Query, int Limit);
+llvm::Expected>
+runDocumentSymbols(ClangdServer , PathRef File);
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -117,5 +117,12 @@
return std::move(*Result);
}
+llvm::Expected>
+runDocumentSymbols(ClangdServer , PathRef File) {
+ llvm::Optional>> Result;
+ Server.documentSymbols(File, capture(Result));
+ return std::move(*Result);
+}
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- unittests/clangd/FindSymbolsTests.cpp
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -22,6 +22,7 @@
using ::testing::AllOf;
using ::testing::AnyOf;
using ::testing::ElementsAre;
+using ::testing::ElementsAreArray;
using ::testing::IsEmpty;
using ::testing::UnorderedElementsAre;
@@ -37,6 +38,7 @@
return (arg.containerName + "::" + arg.name) == Name;
}
MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SymRange, Range, "") { return arg.location.range == Range; }
ClangdServer::Options optsForTests() {
auto ServerOpts = ClangdServer::optsForTest();
@@ -287,5 +289,242 @@
EXPECT_THAT(getSymbols("foo"), ElementsAre(QName("foo")));
}
+namespace {
+class DocumentSymbolsTest : public ::testing::Test {
+public:
+ DocumentSymbolsTest()
+ : Server(CDB, FSProvider, DiagConsumer, optsForTests()) {}
+
+protected:
+ MockFSProvider FSProvider;
+ MockCompilationDatabase CDB;
+ IgnoreDiagnostics DiagConsumer;
+ ClangdServer Server;
+
+ std::vector getSymbols(PathRef File) {
+EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for preamble";
+auto SymbolInfos = runDocumentSymbols(Server, File);
+EXPECT_TRUE(bool(SymbolInfos)) << "documentSymbols returned an error";
+return *SymbolInfos;
+ }
+
+ void addFile(StringRef FilePath, StringRef Contents) {
+FSProvider.Files[FilePath] = Contents;
+Server.addDocument(FilePath, Contents);
+ }
+};
+} // namespace
+
+TEST_F(DocumentSymbolsTest, BasicSymbols) {
+ std::string FilePath = testPath("foo.cpp");
+ addFile(FilePath, R"(
+class Foo;
+class Foo {
+ Foo() {}
+ Foo(int a) {}
+ void f();
+ friend void f1();
+ friend class Friend;
+ Foo& operator=(const Foo&);
+ ~Foo();
+ class Nested {
+ void f();
+ };
+};
+class Friend {
+};
+
+void f1();
+inline void f2() {}
+static const int KInt = 2;
+const char* kStr = "123";
+
+void f1() {}
+
+namespace foo {
+// Type alias
+typedef int int32;
+using int32_t = int32;
+
+// Variable
+int v1;
+
+// Namespace
+namespace bar {
+int v2;
+}
+// Namespace alias
+namespace baz = bar;
+
+// FIXME: using declaration is not supported as the IndexAction will ignore
+// implicit declarations (the implicit using shadow declaration) by default,
+// and there is no way to customize this behavior at the moment.
+using bar::v2;
+} // namespace foo
+ )");
+ EXPECT_THAT(getSymbols(FilePath),
+ ElementsAreArray(
+ {AllOf(QName("Foo"), WithKind(SymbolKind::Class)),
+ AllOf(QName("Foo"), WithKind(SymbolKind::Class)),
+ AllOf(QName("Foo::Foo"), WithKind(SymbolKind::Method)),
+ AllOf(QName("Foo::Foo"), WithKind(SymbolKind::Method)),
+ AllOf(QName("Foo::f"), WithKind(SymbolKind::Method)),
+ AllOf(QName("f1"), WithKind(SymbolKind::Function)),
+ AllOf(QName("Foo::operator="), WithKind(SymbolKind::Method)),
+ AllOf(QName("Foo::~Foo"), WithKind(SymbolKind::Method)),
+ AllOf(QName("Foo::Nested"), WithKind(SymbolKind::Class)),
+ AllOf(QName("Foo::Nested::f"), WithKind(SymbolKind::Method)),
+
+ AllOf(QName("Friend"),
[PATCH] D47846: [clangd] Implementation of textDocument/documentSymbol
malaperle added inline comments.
Comment at: unittests/clangd/FindSymbolsTests.cpp:39
}
+MATCHER_P(QName, Name, "") {
+ if (arg.containerName.empty())
I updated the other tests to use this in https://reviews.llvm.org/D47847
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47846
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47846: [clangd] Implementation of textDocument/documentSymbol
malaperle created this revision.
Herald added subscribers: cfe-commits, jkorous, MaskRay, ioeric, ilya-biryukov.
An AST-based approach is used to retrieve the document symbols rather than an
in-memory index query. The index is not an ideal fit to achieve this because of
the file-centric query being done here whereas the index is suited for
project-wide queries. Document symbols also includes more symbols and need to
keep the order as seen in the file.
Signed-off-by: Marc-Andre Laperle
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47846
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/FindSymbols.cpp
clangd/FindSymbols.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/XRefs.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
test/clangd/symbols.test
unittests/clangd/FindSymbolsTests.cpp
unittests/clangd/SyncAPI.cpp
unittests/clangd/SyncAPI.h
Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -44,6 +44,9 @@
llvm::Expected>
runWorkspaceSymbols(ClangdServer , StringRef Query, int Limit);
+llvm::Expected>
+runDocumentSymbols(ClangdServer , PathRef File);
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -117,5 +117,12 @@
return std::move(*Result);
}
+llvm::Expected>
+runDocumentSymbols(ClangdServer , PathRef File) {
+ llvm::Optional>> Result;
+ Server.documentSymbols(File, capture(Result));
+ return std::move(*Result);
+}
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- unittests/clangd/FindSymbolsTests.cpp
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -22,6 +22,7 @@
using ::testing::AllOf;
using ::testing::AnyOf;
using ::testing::ElementsAre;
+using ::testing::ElementsAreArray;
using ::testing::IsEmpty;
using ::testing::UnorderedElementsAre;
@@ -35,7 +36,13 @@
MATCHER_P(InContainer, ContainerName, "") {
return arg.containerName == ContainerName;
}
+MATCHER_P(QName, Name, "") {
+ if (arg.containerName.empty())
+return arg.name == Name;
+ return (arg.containerName + "::" + arg.name) == Name;
+}
MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SymRange, Range, "") { return arg.location.range == Range; }
ClangdServer::Options optsForTests() {
auto ServerOpts = ClangdServer::optsForTest();
@@ -284,5 +291,244 @@
AllOf(Named("foo2"), InContainer("");
}
+namespace {
+class DocumentSymbolsTest : public ::testing::Test {
+public:
+ DocumentSymbolsTest()
+ : Server(CDB, FSProvider, DiagConsumer, optsForTests()) {}
+
+protected:
+ MockFSProvider FSProvider;
+ MockCompilationDatabase CDB;
+ IgnoreDiagnostics DiagConsumer;
+ ClangdServer Server;
+
+ std::vector getSymbols(PathRef File) {
+EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for preamble";
+auto SymbolInfos = runDocumentSymbols(Server, File);
+EXPECT_TRUE(bool(SymbolInfos)) << "documentSymbols returned an error";
+return *SymbolInfos;
+ }
+
+ void addFile(StringRef FilePath, StringRef Contents) {
+FSProvider.Files[FilePath] = Contents;
+Server.addDocument(FilePath, Contents);
+ }
+};
+} // namespace
+
+TEST_F(DocumentSymbolsTest, BasicSymbols) {
+ std::string FilePath = testPath("foo.cpp");
+ addFile(FilePath, R"(
+class Foo;
+class Foo {
+ Foo() {}
+ Foo(int a) {}
+ void f();
+ friend void f1();
+ friend class Friend;
+ Foo& operator=(const Foo&);
+ ~Foo();
+ class Nested {
+ void f();
+ };
+};
+class Friend {
+};
+
+void f1();
+inline void f2() {}
+static const int KInt = 2;
+const char* kStr = "123";
+
+void f1() {}
+
+namespace foo {
+// Type alias
+typedef int int32;
+using int32_t = int32;
+
+// Variable
+int v1;
+
+// Namespace
+namespace bar {
+int v2;
+}
+// Namespace alias
+namespace baz = bar;
+
+// FIXME: using declaration is not supported as the IndexAction will ignore
+// implicit declarations (the implicit using shadow declaration) by default,
+// and there is no way to customize this behavior at the moment.
+using bar::v2;
+} // namespace foo
+ )");
+ EXPECT_THAT(getSymbols(FilePath),
+ ElementsAreArray(
+ {AllOf(QName("Foo"), WithKind(SymbolKind::Class)),
+ AllOf(QName("Foo"), WithKind(SymbolKind::Class)),
+ AllOf(QName("Foo::Foo"),
