[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol
This revision was automatically updated to reflect the committed changes.
Closed by commit rL347498: [clangd] Add support for hierarchical documentSymbol
(authored by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D52311
Files:
clang-tools-extra/trunk/clangd/AST.cpp
clang-tools-extra/trunk/clangd/AST.h
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/clients/clangd-vscode/package.json
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/clangd/AST.cpp
===
--- clang-tools-extra/trunk/clangd/AST.cpp
+++ clang-tools-extra/trunk/clangd/AST.cpp
@@ -11,9 +11,12 @@
#include "clang/AST/ASTContext.h"
#include "clang/AST/Decl.h"
+#include "clang/AST/DeclTemplate.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Index/USRGeneration.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/ScopedPrinter.h"
using namespace llvm;
namespace clang {
@@ -61,6 +64,46 @@
return QName;
}
+static const TemplateArgumentList *
+getTemplateSpecializationArgs(const NamedDecl ) {
+ if (auto *Func = llvm::dyn_cast())
+return Func->getTemplateSpecializationArgs();
+ if (auto *Cls = llvm::dyn_cast())
+return >getTemplateInstantiationArgs();
+ if (auto *Var = llvm::dyn_cast())
+return >getTemplateInstantiationArgs();
+ return nullptr;
+}
+
+std::string printName(const ASTContext , const NamedDecl ) {
+ std::string Name;
+ llvm::raw_string_ostream Out(Name);
+ PrintingPolicy PP(Ctx.getLangOpts());
+ // Handle 'using namespace'. They all have the same name - .
+ if (auto *UD = llvm::dyn_cast()) {
+Out << "using namespace ";
+if (auto *Qual = UD->getQualifier())
+ Qual->print(Out, PP);
+UD->getNominatedNamespaceAsWritten()->printName(Out);
+return Out.str();
+ }
+ ND.getDeclName().print(Out, PP);
+ if (!Out.str().empty()) {
+// FIXME(ibiryukov): do not show args not explicitly written by the user.
+if (auto *ArgList = getTemplateSpecializationArgs(ND))
+ printTemplateArgumentList(Out, ArgList->asArray(), PP);
+return Out.str();
+ }
+ // The name was empty, so present an anonymous entity.
+ if (auto *NS = llvm::dyn_cast())
+return "(anonymous namespace)";
+ if (auto *Cls = llvm::dyn_cast())
+return ("(anonymous " + Cls->getKindName() + ")").str();
+ if (auto *En = llvm::dyn_cast())
+return "(anonymous enum)";
+ return "(anonymous)";
+}
+
std::string printNamespaceScope(const DeclContext ) {
for (const auto *Ctx = Ctx != nullptr; Ctx = Ctx->getParent())
if (const auto *NS = dyn_cast(Ctx))
Index: clang-tools-extra/trunk/clangd/ClangdServer.h
===
--- clang-tools-extra/trunk/clangd/ClangdServer.h
+++ clang-tools-extra/trunk/clangd/ClangdServer.h
@@ -167,7 +167,7 @@
/// Retrieve the symbols within the specified file.
void documentSymbols(StringRef File,
- Callback> CB);
+ Callback> CB);
/// Retrieve locations for symbol references.
void findReferences(PathRef File, Position Pos,
Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.h
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.h
@@ -66,8 +66,11 @@
Callback>);
void onDocumentFormatting(const DocumentFormattingParams &,
Callback>);
+ // The results are serialized 'vector' if
+ // SupportsHierarchicalDocumentSymbol is true and 'vector'
+ // otherwise.
void onDocumentSymbol(const DocumentSymbolParams &,
-Callback>);
+Callback);
void onCodeAction(const CodeActionParams &, Callback);
void onCompletion(const TextDocumentPositionParams &,
Callback);
@@ -128,6 +131,8 @@
CompletionItemKindBitset SupportedCompletionItemKinds;
// Whether the client supports CodeAction response objects.
bool SupportsCodeAction = false;
+ /// From capabilities of textDocument/documentSymbol.
+ bool SupportsHierarchicalDocumentSymbol = false;
// Store of the current versions of the open documents.
DraftStore DraftMgr;
Index: clang-tools-extra/trunk/clangd/AST.h
[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol
ilya-biryukov updated this revision to Diff 175130.
ilya-biryukov added a comment.
- Handle 'using namespace' in printName
- Do not mix-in symbols with locations from other files.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52311
Files:
clangd/AST.cpp
clangd/AST.h
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/clients/clangd-vscode/package.json
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
@@ -47,8 +47,8 @@
llvm::Expected>
runWorkspaceSymbols(ClangdServer , StringRef Query, int Limit);
-llvm::Expected>
-runDocumentSymbols(ClangdServer , PathRef File);
+Expected> runDocumentSymbols(ClangdServer ,
+ PathRef File);
SymbolSlab runFuzzyFind(const SymbolIndex , StringRef Query);
SymbolSlab runFuzzyFind(const SymbolIndex , const FuzzyFindRequest );
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -120,9 +120,9 @@
return std::move(*Result);
}
-Expected>
-runDocumentSymbols(ClangdServer , PathRef File) {
- Optional>> Result;
+Expected> runDocumentSymbols(ClangdServer ,
+ PathRef File) {
+ Optional>> Result;
Server.documentSymbols(File, capture(Result));
return std::move(*Result);
}
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- unittests/clangd/FindSymbolsTests.cpp
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -14,6 +14,8 @@
#include "gmock/gmock.h"
#include "gtest/gtest.h"
+using namespace llvm;
+
namespace clang {
namespace clangd {
@@ -23,6 +25,7 @@
using ::testing::AnyOf;
using ::testing::ElementsAre;
using ::testing::ElementsAreArray;
+using ::testing::Field;
using ::testing::IsEmpty;
using ::testing::UnorderedElementsAre;
@@ -37,9 +40,17 @@
return arg.name == Name;
return (arg.containerName + "::" + arg.name) == Name;
}
+MATCHER_P(WithName, N, "") { return arg.name == N; }
MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
MATCHER_P(SymRange, Range, "") { return arg.location.range == Range; }
+// GMock helpers for matching DocumentSymbol.
+MATCHER_P(SymNameRange, Range, "") { return arg.selectionRange == Range; }
+template
+testing::Matcher Children(ChildMatchers... ChildrenM) {
+ return Field(::children, ElementsAre(ChildrenM...));
+}
+
ClangdServer::Options optsForTests() {
auto ServerOpts = ClangdServer::optsForTest();
ServerOpts.WorkspaceRoot = testRoot();
@@ -300,7 +311,7 @@
IgnoreDiagnostics DiagConsumer;
ClangdServer Server;
- std::vector getSymbols(PathRef File) {
+ 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";
@@ -363,31 +374,46 @@
)");
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)),
- AllOf(QName("Foo::Nested::f"), WithKind(SymbolKind::Method)),
- AllOf(QName("Friend"), WithKind(SymbolKind::Class)),
- AllOf(QName("f1"), WithKind(SymbolKind::Function)),
- AllOf(QName("f2"), WithKind(SymbolKind::Function)),
- AllOf(QName("KInt"), WithKind(SymbolKind::Variable)),
- AllOf(QName("kStr"), WithKind(SymbolKind::Variable)),
- AllOf(QName("f1"), WithKind(SymbolKind::Function)),
- AllOf(QName("foo"), WithKind(SymbolKind::Namespace)),
- AllOf(QName("foo::int32"), WithKind(SymbolKind::Class)),
- AllOf(QName("foo::int32_t"), WithKind(SymbolKind::Class)),
- AllOf(QName("foo::v1"), WithKind(SymbolKind::Variable)),
-
[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol
ilya-biryukov added inline comments.
Comment at: unittests/clangd/FindSymbolsTests.cpp:442
+SymNameRange(Main.range("decl"),
+ AllOf(WithName("f"), WithKind(SymbolKind::Method),
+SymNameRange(Main.range("def");
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > this one is to be fixed, right?
> > Why? The outline view gives both the declaration and the definition, since
> > both were written in the code.
> > That seems to be in line with what I'd expect from the outline view.
> Sorry, I meant the name should be `Foo::def` instead of `def`, which you
> mentioned in a previous comment.
>
> OK to land without this, but I think it deserves a fixme.
> (If you think this is the *right* behavior, let's discuss further!)
Oh, yeah, that I agree with! Will look into fixing this in a follow-up change.
Comment at: unittests/clangd/FindSymbolsTests.cpp:521
+ChildrenAre(AllOf(WithName("x"),
WithKind(SymbolKind::Field,
+ AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct),
+ChildrenAre(WithName("y"))),
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > hmm, this seems pretty confusing - I think `Tmpl` would be a
> > > clearer name for a specialization, even if we just have `Tmpl` for the
> > > primary template.
> > > Partial specializations are confusing, though :-/
> > Done. Now prints as `Tmpl`.
> > This may also include some arguments not written by the users (e.g. some
> > default args), but added a FIXME to fix this, it's not entirely trivial
> Nice! I thought this was going to be really hard.
>
> (You can specialize without naming the defaulted args? That sounds...
> obscure. Definitely fine to leave for later)
Yeah, you can. It's especially common with function where specializations don't
name any arguments and those are inferred from the function type...
Comment at: unittests/clangd/FindSymbolsTests.cpp:523
+ChildrenAre(WithName("y"))),
+ AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct), NoChildren()),
+ AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct),
NoChildren(;
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > why the change in policy with this patch? (one of these previously was
> > > deliberately not indexed, now is)
> > We might have different goals in mind here.
> > From my perspective, the purpose of the outline is to cover all things
> > written in the code: there are 4 decls that the user wrote: one primary
> > template, one template specialization and two template instantiations.
> >
> > What is your model here?
> I don't have a particular opinion, just wondering if this was a deliberate
> change or fell out of the implementation.
>
> (The old behavior was tested, so I guess Marc-Andre had a reason, but it
> didn't come up in the review. I don't think this is common enough to worry
> much about either way)
>
Yeah, this fell out of the implementation of walking over the "user-written"
part of the AST and it looked sensible so I updated the test.
Comment at: unittests/clangd/FindSymbolsTests.cpp:571
addFile(FilePath, R"(
enum {
Red
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > hmm, our handling of anonymous enums seems different than anonymous
> > > namespaces - why?
> > No rigorous reasons. The namespaces tend to group more things together, so
> > it's arguably more useful to have an option of folding their subitems in
> > the tree.
> > Anonymous enums are typically used only in the classes (and on the
> > top-level in C to define constants?) and I feel having an extra node there
> > merely produces noise.
> > Happy to reconsider, what's your preferred behavior?
> I *think* I might prefer the opposite - namespaces flat and enums grouped :-)
> I'm not certain nor adamant about either, so consider my thoughts below, and
> pick something.
>
> **Namespaces**: anonymous namespaces are mostly (entirely?) used to control
> visibility. I don't think they *group* in a meaningful way, they just hide
> things like `static` does. In a perfect world I think there'd be a "private"
> bit on outline elements that was shown in the UI, and we'd put it on both
> things-in-anon-namespaces and static-as-in-private decls. The main
> counterargument I can see: because we typically don't indent inside
> namespaces, the enclosing anonymous namespace can be hard to recognize and
> jump to.
>
> **Enums**: conversely, I think these _typically_ do group their elements, and
> there's often a strong semantic boundary between the last entry in the enum
> and the first decl after it. When people use a namespacing prefix (C-style
> VK_Foo, VK_Bar) then it's visually
[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol
ilya-biryukov updated this revision to Diff 175116.
ilya-biryukov added a comment.
- Do not drop anonymous enums
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52311
Files:
clangd/AST.cpp
clangd/AST.h
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/clients/clangd-vscode/package.json
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
@@ -47,8 +47,8 @@
llvm::Expected>
runWorkspaceSymbols(ClangdServer , StringRef Query, int Limit);
-llvm::Expected>
-runDocumentSymbols(ClangdServer , PathRef File);
+Expected> runDocumentSymbols(ClangdServer ,
+ PathRef File);
SymbolSlab runFuzzyFind(const SymbolIndex , StringRef Query);
SymbolSlab runFuzzyFind(const SymbolIndex , const FuzzyFindRequest );
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -120,9 +120,9 @@
return std::move(*Result);
}
-Expected>
-runDocumentSymbols(ClangdServer , PathRef File) {
- Optional>> Result;
+Expected> runDocumentSymbols(ClangdServer ,
+ PathRef File) {
+ Optional>> Result;
Server.documentSymbols(File, capture(Result));
return std::move(*Result);
}
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- unittests/clangd/FindSymbolsTests.cpp
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -23,6 +23,7 @@
using ::testing::AnyOf;
using ::testing::ElementsAre;
using ::testing::ElementsAreArray;
+using ::testing::Field;
using ::testing::IsEmpty;
using ::testing::UnorderedElementsAre;
@@ -37,9 +38,17 @@
return arg.name == Name;
return (arg.containerName + "::" + arg.name) == Name;
}
+MATCHER_P(WithName, N, "") { return arg.name == N; }
MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
MATCHER_P(SymRange, Range, "") { return arg.location.range == Range; }
+// GMock helpers for matching DocumentSymbol.
+MATCHER_P(SymNameRange, Range, "") { return arg.selectionRange == Range; }
+template
+testing::Matcher Children(ChildMatchers... ChildrenM) {
+ return Field(::children, ElementsAre(ChildrenM...));
+}
+
ClangdServer::Options optsForTests() {
auto ServerOpts = ClangdServer::optsForTest();
ServerOpts.WorkspaceRoot = testRoot();
@@ -300,7 +309,7 @@
IgnoreDiagnostics DiagConsumer;
ClangdServer Server;
- std::vector getSymbols(PathRef File) {
+ 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";
@@ -363,31 +372,46 @@
)");
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)),
- AllOf(QName("Foo::Nested::f"), WithKind(SymbolKind::Method)),
- AllOf(QName("Friend"), WithKind(SymbolKind::Class)),
- AllOf(QName("f1"), WithKind(SymbolKind::Function)),
- AllOf(QName("f2"), WithKind(SymbolKind::Function)),
- AllOf(QName("KInt"), WithKind(SymbolKind::Variable)),
- AllOf(QName("kStr"), WithKind(SymbolKind::Variable)),
- AllOf(QName("f1"), WithKind(SymbolKind::Function)),
- AllOf(QName("foo"), WithKind(SymbolKind::Namespace)),
- AllOf(QName("foo::int32"), WithKind(SymbolKind::Class)),
- AllOf(QName("foo::int32_t"), WithKind(SymbolKind::Class)),
- AllOf(QName("foo::v1"), WithKind(SymbolKind::Variable)),
- AllOf(QName("foo::bar"), WithKind(SymbolKind::Namespace)),
- AllOf(QName("foo::bar::v2"), WithKind(SymbolKind::Variable)),
- AllOf(QName("foo::baz"),
[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: unittests/clangd/FindSymbolsTests.cpp:442
+SymNameRange(Main.range("decl"),
+ AllOf(WithName("f"), WithKind(SymbolKind::Method),
+SymNameRange(Main.range("def");
ilya-biryukov wrote:
> sammccall wrote:
> > this one is to be fixed, right?
> Why? The outline view gives both the declaration and the definition, since
> both were written in the code.
> That seems to be in line with what I'd expect from the outline view.
Sorry, I meant the name should be `Foo::def` instead of `def`, which you
mentioned in a previous comment.
OK to land without this, but I think it deserves a fixme.
(If you think this is the *right* behavior, let's discuss further!)
Comment at: unittests/clangd/FindSymbolsTests.cpp:521
+ChildrenAre(AllOf(WithName("x"),
WithKind(SymbolKind::Field,
+ AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct),
+ChildrenAre(WithName("y"))),
ilya-biryukov wrote:
> sammccall wrote:
> > hmm, this seems pretty confusing - I think `Tmpl` would be a clearer
> > name for a specialization, even if we just have `Tmpl` for the primary
> > template.
> > Partial specializations are confusing, though :-/
> Done. Now prints as `Tmpl`.
> This may also include some arguments not written by the users (e.g. some
> default args), but added a FIXME to fix this, it's not entirely trivial
Nice! I thought this was going to be really hard.
(You can specialize without naming the defaulted args? That sounds... obscure.
Definitely fine to leave for later)
Comment at: unittests/clangd/FindSymbolsTests.cpp:523
+ChildrenAre(WithName("y"))),
+ AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct), NoChildren()),
+ AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct),
NoChildren(;
ilya-biryukov wrote:
> sammccall wrote:
> > why the change in policy with this patch? (one of these previously was
> > deliberately not indexed, now is)
> We might have different goals in mind here.
> From my perspective, the purpose of the outline is to cover all things
> written in the code: there are 4 decls that the user wrote: one primary
> template, one template specialization and two template instantiations.
>
> What is your model here?
I don't have a particular opinion, just wondering if this was a deliberate
change or fell out of the implementation.
(The old behavior was tested, so I guess Marc-Andre had a reason, but it didn't
come up in the review. I don't think this is common enough to worry much about
either way)
Comment at: unittests/clangd/FindSymbolsTests.cpp:571
addFile(FilePath, R"(
enum {
Red
ilya-biryukov wrote:
> sammccall wrote:
> > hmm, our handling of anonymous enums seems different than anonymous
> > namespaces - why?
> No rigorous reasons. The namespaces tend to group more things together, so
> it's arguably more useful to have an option of folding their subitems in the
> tree.
> Anonymous enums are typically used only in the classes (and on the top-level
> in C to define constants?) and I feel having an extra node there merely
> produces noise.
> Happy to reconsider, what's your preferred behavior?
I *think* I might prefer the opposite - namespaces flat and enums grouped :-)
I'm not certain nor adamant about either, so consider my thoughts below, and
pick something.
**Namespaces**: anonymous namespaces are mostly (entirely?) used to control
visibility. I don't think they *group* in a meaningful way, they just hide
things like `static` does. In a perfect world I think there'd be a "private"
bit on outline elements that was shown in the UI, and we'd put it on both
things-in-anon-namespaces and static-as-in-private decls. The main
counterargument I can see: because we typically don't indent inside namespaces,
the enclosing anonymous namespace can be hard to recognize and jump to.
**Enums**: conversely, I think these _typically_ do group their elements, and
there's often a strong semantic boundary between the last entry in the enum and
the first decl after it. When people use a namespacing prefix (C-style VK_Foo,
VK_Bar) then it's visually noticeable, but this is far from universal in C++
(particularly at class scope).
**Consistency**: It's not *really* confusing if these are different, just
*slightly* confusing. Anonymous structs etc must certainly be grouped, so this
is a (weak) argument for grouping everything.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52311
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol
ilya-biryukov updated this revision to Diff 174960.
ilya-biryukov added a comment.
- Remove accidentally added qualifiers
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52311
Files:
clangd/AST.cpp
clangd/AST.h
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/clients/clangd-vscode/package.json
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
@@ -47,7 +47,7 @@
llvm::Expected>
runWorkspaceSymbols(ClangdServer , StringRef Query, int Limit);
-llvm::Expected>
+Expected>
runDocumentSymbols(ClangdServer , PathRef File);
SymbolSlab runFuzzyFind(const SymbolIndex , StringRef Query);
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -120,9 +120,9 @@
return std::move(*Result);
}
-Expected>
+Expected>
runDocumentSymbols(ClangdServer , PathRef File) {
- Optional>> Result;
+ Optional>> Result;
Server.documentSymbols(File, capture(Result));
return std::move(*Result);
}
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- unittests/clangd/FindSymbolsTests.cpp
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -23,6 +23,7 @@
using ::testing::AnyOf;
using ::testing::ElementsAre;
using ::testing::ElementsAreArray;
+using ::testing::Field;
using ::testing::IsEmpty;
using ::testing::UnorderedElementsAre;
@@ -37,9 +38,17 @@
return arg.name == Name;
return (arg.containerName + "::" + arg.name) == Name;
}
+MATCHER_P(WithName, N, "") { return arg.name == N; }
MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
MATCHER_P(SymRange, Range, "") { return arg.location.range == Range; }
+// GMock helpers for matching DocumentSymbol.
+MATCHER_P(SymNameRange, Range, "") { return arg.selectionRange == Range; }
+template
+testing::Matcher Children(ChildMatchers... ChildrenM) {
+ return Field(::children, ElementsAre(ChildrenM...));
+}
+
ClangdServer::Options optsForTests() {
auto ServerOpts = ClangdServer::optsForTest();
ServerOpts.WorkspaceRoot = testRoot();
@@ -301,7 +310,7 @@
IgnoreDiagnostics DiagConsumer;
ClangdServer Server;
- std::vector getSymbols(PathRef File) {
+ 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";
@@ -364,31 +373,46 @@
)");
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)),
- AllOf(QName("Foo::Nested::f"), WithKind(SymbolKind::Method)),
- AllOf(QName("Friend"), WithKind(SymbolKind::Class)),
- AllOf(QName("f1"), WithKind(SymbolKind::Function)),
- AllOf(QName("f2"), WithKind(SymbolKind::Function)),
- AllOf(QName("KInt"), WithKind(SymbolKind::Variable)),
- AllOf(QName("kStr"), WithKind(SymbolKind::Variable)),
- AllOf(QName("f1"), WithKind(SymbolKind::Function)),
- AllOf(QName("foo"), WithKind(SymbolKind::Namespace)),
- AllOf(QName("foo::int32"), WithKind(SymbolKind::Class)),
- AllOf(QName("foo::int32_t"), WithKind(SymbolKind::Class)),
- AllOf(QName("foo::v1"), WithKind(SymbolKind::Variable)),
- AllOf(QName("foo::bar"), WithKind(SymbolKind::Namespace)),
- AllOf(QName("foo::bar::v2"), WithKind(SymbolKind::Variable)),
- AllOf(QName("foo::baz"), WithKind(SymbolKind::Namespace))}));
+ EXPECT_THAT(
+ getSymbols(FilePath),
+ ElementsAreArray(
+ {AllOf(WithName("Foo"), WithKind(SymbolKind::Class), Children()),
+ AllOf(WithName("Foo"), WithKind(SymbolKind::Class),
+
[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol
ilya-biryukov added inline comments.
Comment at: clangd/FindSymbols.cpp:190
+ index::SymbolInfo SymInfo = index::getSymbolInfo();
+ SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
sammccall wrote:
> may want to add a FIXME here: per the tests, it's not classifying
> constructor/destructor/operator correctly (they're all "methods").
>
> cons/dest is just indexSymbolKindToSymbolKind needing an update, but operator
> isn't represented in index::SymbolKind.
> Maybe we should stop using index::SymbolKind entirely, it doesn't appear to
> be great.
Added a FIXME.
From what I can tell, writing our own classification function shouldn't be too
much work and would clearly fix those cases. We should do this!
Comment at: clangd/FindSymbols.cpp:208
+std::vector collectDocSymbols(ParsedAST ) {
+ struct CollectSymbols {
+CollectSymbols(ParsedAST , std::vector )
sammccall wrote:
> sammccall wrote:
> > sammccall wrote:
> > > this class seems maybe too long to live inside this function. (I wouldn't
> > > worry about it being visible to other stuff, the file is small and
> > > focused)
> > (this looks more like a class than a struct?)
> should this be a RecursiveASTVisitor?
> I guess not, but please explain why (there's no documentation on the
> implementation strategy, and it's complicated)
Added a comment.
Comment at: clangd/FindSymbols.cpp:264
+// ignored.
+if (auto *Info = Func->getTemplateSpecializationInfo()) {
+ if (!Info->isExplicitInstantiationOrSpecialization())
sammccall wrote:
> isn't this covered by D->isImplicit?
Nope, IIUC things are only marked "implicit" in clangd if they were generated
by the compiler from the start.
For this case, the **initial** template code was written by the user, so
`isImplicit()` returns false for both the template and its instantiations.
Comment at: clangd/FindSymbols.cpp:276
+ // children.
+ // - implicit instantiations, i.e. not written by the user.
+ // Do not visit at all, they are not present in the code.
sammccall wrote:
> isn't this covered by D->isImplicit() above?
See the other comment.
Comment at: clangd/Protocol.h:43
InvalidParams = -32602,
+
InternalError = -32603,
sammccall wrote:
> why?
Sorry, accidental change. Reverted.
Comment at: unittests/clangd/FindSymbolsTests.cpp:442
+SymNameRange(Main.range("decl"),
+ AllOf(WithName("f"), WithKind(SymbolKind::Method),
+SymNameRange(Main.range("def");
sammccall wrote:
> this one is to be fixed, right?
Why? The outline view gives both the declaration and the definition, since both
were written in the code.
That seems to be in line with what I'd expect from the outline view.
Comment at: unittests/clangd/FindSymbolsTests.cpp:521
+ChildrenAre(AllOf(WithName("x"),
WithKind(SymbolKind::Field,
+ AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct),
+ChildrenAre(WithName("y"))),
sammccall wrote:
> hmm, this seems pretty confusing - I think `Tmpl` would be a clearer
> name for a specialization, even if we just have `Tmpl` for the primary
> template.
> Partial specializations are confusing, though :-/
Done. Now prints as `Tmpl`.
This may also include some arguments not written by the users (e.g. some
default args), but added a FIXME to fix this, it's not entirely trivial
Comment at: unittests/clangd/FindSymbolsTests.cpp:523
+ChildrenAre(WithName("y"))),
+ AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct), NoChildren()),
+ AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct),
NoChildren(;
sammccall wrote:
> why the change in policy with this patch? (one of these previously was
> deliberately not indexed, now is)
We might have different goals in mind here.
From my perspective, the purpose of the outline is to cover all things written
in the code: there are 4 decls that the user wrote: one primary template, one
template specialization and two template instantiations.
What is your model here?
Comment at: unittests/clangd/FindSymbolsTests.cpp:571
addFile(FilePath, R"(
enum {
Red
sammccall wrote:
> hmm, our handling of anonymous enums seems different than anonymous
> namespaces - why?
No rigorous reasons. The namespaces tend to group more things together, so it's
arguably more useful to have an option of folding their subitems in the tree.
Anonymous enums are typically used only in the classes (and on the top-level in
C to define constants?) and I feel having an extra node there merely produces
noise.
Happy to reconsider,
[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol
ilya-biryukov updated this revision to Diff 174958.
ilya-biryukov marked 15 inline comments as done.
ilya-biryukov added a comment.
- Address comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52311
Files:
clangd/AST.cpp
clangd/AST.h
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/clients/clangd-vscode/package.json
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
@@ -47,7 +47,7 @@
llvm::Expected>
runWorkspaceSymbols(ClangdServer , StringRef Query, int Limit);
-llvm::Expected>
+llvm::Expected>
runDocumentSymbols(ClangdServer , PathRef File);
SymbolSlab runFuzzyFind(const SymbolIndex , StringRef Query);
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -120,9 +120,9 @@
return std::move(*Result);
}
-Expected>
+llvm::Expected>
runDocumentSymbols(ClangdServer , PathRef File) {
- Optional>> Result;
+ llvm::Optional>> Result;
Server.documentSymbols(File, capture(Result));
return std::move(*Result);
}
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- unittests/clangd/FindSymbolsTests.cpp
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -23,6 +23,7 @@
using ::testing::AnyOf;
using ::testing::ElementsAre;
using ::testing::ElementsAreArray;
+using ::testing::Field;
using ::testing::IsEmpty;
using ::testing::UnorderedElementsAre;
@@ -37,9 +38,17 @@
return arg.name == Name;
return (arg.containerName + "::" + arg.name) == Name;
}
+MATCHER_P(WithName, N, "") { return arg.name == N; }
MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
MATCHER_P(SymRange, Range, "") { return arg.location.range == Range; }
+// GMock helpers for matching DocumentSymbol.
+MATCHER_P(SymNameRange, Range, "") { return arg.selectionRange == Range; }
+template
+testing::Matcher Children(ChildMatchers... ChildrenM) {
+ return Field(::children, ElementsAre(ChildrenM...));
+}
+
ClangdServer::Options optsForTests() {
auto ServerOpts = ClangdServer::optsForTest();
ServerOpts.WorkspaceRoot = testRoot();
@@ -301,7 +310,7 @@
IgnoreDiagnostics DiagConsumer;
ClangdServer Server;
- std::vector getSymbols(PathRef File) {
+ 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";
@@ -364,31 +373,46 @@
)");
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)),
- AllOf(QName("Foo::Nested::f"), WithKind(SymbolKind::Method)),
- AllOf(QName("Friend"), WithKind(SymbolKind::Class)),
- AllOf(QName("f1"), WithKind(SymbolKind::Function)),
- AllOf(QName("f2"), WithKind(SymbolKind::Function)),
- AllOf(QName("KInt"), WithKind(SymbolKind::Variable)),
- AllOf(QName("kStr"), WithKind(SymbolKind::Variable)),
- AllOf(QName("f1"), WithKind(SymbolKind::Function)),
- AllOf(QName("foo"), WithKind(SymbolKind::Namespace)),
- AllOf(QName("foo::int32"), WithKind(SymbolKind::Class)),
- AllOf(QName("foo::int32_t"), WithKind(SymbolKind::Class)),
- AllOf(QName("foo::v1"), WithKind(SymbolKind::Variable)),
- AllOf(QName("foo::bar"), WithKind(SymbolKind::Namespace)),
- AllOf(QName("foo::bar::v2"), WithKind(SymbolKind::Variable)),
- AllOf(QName("foo::baz"), WithKind(SymbolKind::Namespace))}));
+ EXPECT_THAT(
+ getSymbols(FilePath),
+ ElementsAreArray(
+ {AllOf(WithName("Foo"), WithKind(SymbolKind::Class), Children()),
+ AllOf(WithName("Foo"),
[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol
sammccall added a comment.
Very nice! Mostly just a few style/structure nits.
Comment at: clangd/AST.cpp:69
+std::string printName(const NamedDecl ) {
+ const NamedDecl *NameSource =
+ std::string Name = llvm::to_string(NameSource->getDeclName());
just use ND?
Comment at: clangd/ClangdLSPServer.cpp:28
+/// Used by the clients that do not support the hierarchical view.
+static std::vector
+flattenDocumentSymbols(llvm::ArrayRef Symbols,
move this nearer the call site?
Comment at: clangd/ClangdLSPServer.cpp:29
+static std::vector
+flattenDocumentSymbols(llvm::ArrayRef Symbols,
+ const URIForFile ) {
or flattenSymbolHierarchy?
Comment at: clangd/ClangdLSPServer.cpp:31
+ const URIForFile ) {
+ // A helper struct to keep the state for recursive processing, computes
+ // results on construction.
auto-typed lambdas can't be recursive, but std::function shouldn't be too slow
here? And clearer I think:
```
vector Results;
std::function Process = [&](const
DocumentSymbol , StringRef ParentName) {
...
};
for (const auto& TopLevel : Symbols)
Process(TopLevel);
return Results;
```
failing that, I found this pattern confusing - I'd suggest either making it a
recursive plain function, or a standard functor object as if it were a lambda
(with the recursive call being `operator()`)
Comment at: clangd/ClangdLSPServer.cpp:44
+ private:
+void process(const DocumentSymbol , llvm::StringRef ParentName) {
+ SymbolInformation SI;
nit: optional for parent name seems slightly neater
Comment at: clangd/FindSymbols.cpp:190
+ index::SymbolInfo SymInfo = index::getSymbolInfo();
+ SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
may want to add a FIXME here: per the tests, it's not classifying
constructor/destructor/operator correctly (they're all "methods").
cons/dest is just indexSymbolKindToSymbolKind needing an update, but operator
isn't represented in index::SymbolKind.
Maybe we should stop using index::SymbolKind entirely, it doesn't appear to be
great.
Comment at: clangd/FindSymbols.cpp:208
+std::vector collectDocSymbols(ParsedAST ) {
+ struct CollectSymbols {
+CollectSymbols(ParsedAST , std::vector )
this class seems maybe too long to live inside this function. (I wouldn't worry
about it being visible to other stuff, the file is small and focused)
Comment at: clangd/FindSymbols.cpp:208
+std::vector collectDocSymbols(ParsedAST ) {
+ struct CollectSymbols {
+CollectSymbols(ParsedAST , std::vector )
sammccall wrote:
> this class seems maybe too long to live inside this function. (I wouldn't
> worry about it being visible to other stuff, the file is small and focused)
(this looks more like a class than a struct?)
Comment at: clangd/FindSymbols.cpp:208
+std::vector collectDocSymbols(ParsedAST ) {
+ struct CollectSymbols {
+CollectSymbols(ParsedAST , std::vector )
sammccall wrote:
> sammccall wrote:
> > this class seems maybe too long to live inside this function. (I wouldn't
> > worry about it being visible to other stuff, the file is small and focused)
> (this looks more like a class than a struct?)
should this be a RecursiveASTVisitor?
I guess not, but please explain why (there's no documentation on the
implementation strategy, and it's complicated)
Comment at: clangd/FindSymbols.cpp:211
+: Ctx(AST.getASTContext()) {
+ for (auto : AST.getLocalTopLevelDecls())
+traverseDecl(TopLevel, Results);
again, doing the work in the constructor is a bit odd - own the data and expose
it?
Comment at: clangd/FindSymbols.cpp:253
-SymbolInformation SI;
-SI.name = Name;
-SI.kind = SK;
-SI.location = L;
-SI.containerName = Scope;
-Symbols.push_back(std::move(SI));
-return true;
- }
-};
-} // namespace
+VisitKind shouldVisit(Decl* D) {
+ if (D->isImplicit())
we only consider visiting named decls, maybe reflect that here?
(not needed now but may allow future refinement)
Comment at: clangd/FindSymbols.cpp:264
+// ignored.
+if (auto *Info = Func->getTemplateSpecializationInfo()) {
+ if (!Info->isExplicitInstantiationOrSpecialization())
isn't this covered by D->isImplicit?
Comment at: clangd/FindSymbols.cpp:276
+ // children.
+ // - implicit instantiations, i.e. not written by the user.
+ // Do not visit at all, they are not present in the code.
isn't this covered by D->isImplicit() above?
Comment at: clangd/Protocol.h:43
InvalidParams = -32602,
+
[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol
ilya-biryukov added a comment. Another annoyance to fix: - 'using namespace' are shown as '' Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52311 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol
ilya-biryukov updated this revision to Diff 171067.
ilya-biryukov added a comment.
- Improve traversal of the AST.
- Update the tests.
- Add a fallback to SymbolInformation (flat structure) if the client does not
support the hierarhical reponse.
Still some work left to do:
- Do not drop the qualifiers for out-of-line declarations, i.e. 'X::foo' is
presented as 'foo' now, which is a bit confusing.
- See why some names pop up at the global scope (GTest generates macros like
that).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52311
Files:
clangd/AST.cpp
clangd/AST.h
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/clients/clangd-vscode/package.json
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
@@ -47,7 +47,7 @@
llvm::Expected>
runWorkspaceSymbols(ClangdServer , StringRef Query, int Limit);
-llvm::Expected>
+llvm::Expected>
runDocumentSymbols(ClangdServer , PathRef File);
SymbolSlab runFuzzyFind(const SymbolIndex , StringRef Query);
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -119,9 +119,9 @@
return std::move(*Result);
}
-Expected>
+llvm::Expected>
runDocumentSymbols(ClangdServer , PathRef File) {
- Optional>> Result;
+ llvm::Optional>> Result;
Server.documentSymbols(File, capture(Result));
return std::move(*Result);
}
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- unittests/clangd/FindSymbolsTests.cpp
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -23,6 +23,7 @@
using ::testing::AnyOf;
using ::testing::ElementsAre;
using ::testing::ElementsAreArray;
+using ::testing::Field;
using ::testing::IsEmpty;
using ::testing::UnorderedElementsAre;
@@ -37,9 +38,22 @@
return arg.name == Name;
return (arg.containerName + "::" + arg.name) == Name;
}
+MATCHER_P(WithName, N, "") { return arg.name == N; }
MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
MATCHER_P(SymRange, Range, "") { return arg.location.range == Range; }
+// GMock helpers for matching DocumentSymbol.
+MATCHER_P(SymNameRange, Range, "") { return arg.selectionRange == Range; }
+testing::Matcher
+Children(testing::Matcher> ChildrenM) {
+ return Field(::children, ChildrenM);
+}
+template
+testing::Matcher ChildrenAre(ChildMatchers... ChildrenM) {
+ return Children(ElementsAre(ChildrenM...));
+}
+testing::Matcher NoChildren() { return Children(IsEmpty()); }
+
ClangdServer::Options optsForTests() {
auto ServerOpts = ClangdServer::optsForTest();
ServerOpts.WorkspaceRoot = testRoot();
@@ -301,7 +315,7 @@
IgnoreDiagnostics DiagConsumer;
ClangdServer Server;
- std::vector getSymbols(PathRef File) {
+ 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";
@@ -364,31 +378,49 @@
)");
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)),
- AllOf(QName("Foo::Nested::f"), WithKind(SymbolKind::Method)),
- AllOf(QName("Friend"), WithKind(SymbolKind::Class)),
- AllOf(QName("f1"), WithKind(SymbolKind::Function)),
- AllOf(QName("f2"), WithKind(SymbolKind::Function)),
- AllOf(QName("KInt"), WithKind(SymbolKind::Variable)),
- AllOf(QName("kStr"), WithKind(SymbolKind::Variable)),
- AllOf(QName("f1"), WithKind(SymbolKind::Function)),
- AllOf(QName("foo"), WithKind(SymbolKind::Namespace)),
- AllOf(QName("foo::int32"), WithKind(SymbolKind::Class)),
- AllOf(QName("foo::int32_t"), WithKind(SymbolKind::Class)),
-
[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol
ilya-biryukov added a comment. In https://reviews.llvm.org/D52311#1255876, @simark wrote: > I just tried this, this looks very promising! It should help build our > outline view in a much more robust way than we do currently. > A nit for the final patch, I would suggest omitting the fields that are > optional, like `children` (when the list is empty) and `deprecated`. SG, will do. > In vscode, is there a way to get a tree representation of this data? When I > look at "Go to symbol in File..." (ctrl-shift-o) or the outline view at the > bottom of the file explorer, they are both a flat list. What difference does > this patch make in how vscode shows the data? There's an outline view that shows the tree after this patch. IIRC, the "Go to symbol in File..." also shows the tree view now without the lack of functionality like filtering by name, etc. Overall, the experience with a tree seemed strictly better in all scenarios for me. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52311 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol
simark added a comment. I just tried this, this looks very promising! It should help build our outline view in a much more robust way than we do currently. A nit for the final patch, I would suggest omitting the fields that are optional, like `children` (when the list is empty) and `deprecated`. In vscode, is there a way to get a tree representation of this data? When I look at "Go to symbol in File..." (ctrl-shift-o) or the outline view at the bottom of the file explorer, they are both a flat list. What difference does this patch make in how vscode shows the data? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52311 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol
simark added a comment. Ohh awesome, I didn't know the LSP supported that. I'll try it it Theia when I have time. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52311 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol
ilya-biryukov planned changes to this revision. ilya-biryukov added a comment. Posted to make sure it's visible that I've started doing this. Still need to update the tests and check for the capability from the client (and fallback to SymbolInformation if client does not support the new implementation). Nevertheless, it works and looks really good in VSCode. Will update the thread when the code is ready for review. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52311 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol
ilya-biryukov created this revision.
ilya-biryukov added reviewers: ioeric, sammccall, simark.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52311
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/FindSymbols.cpp
clangd/FindSymbols.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/clients/clangd-vscode/package.json
unittests/clangd/SyncAPI.cpp
unittests/clangd/SyncAPI.h
Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -47,7 +47,7 @@
llvm::Expected>
runWorkspaceSymbols(ClangdServer , StringRef Query, int Limit);
-llvm::Expected>
+llvm::Expected>
runDocumentSymbols(ClangdServer , PathRef File);
} // namespace clangd
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -118,7 +118,7 @@
return std::move(*Result);
}
-llvm::Expected>
+llvm::Expected>
runDocumentSymbols(ClangdServer , PathRef File) {
llvm::Optional>> Result;
Server.documentSymbols(File, capture(Result));
Index: clangd/clients/clangd-vscode/package.json
===
--- clangd/clients/clangd-vscode/package.json
+++ clangd/clients/clangd-vscode/package.json
@@ -6,7 +6,7 @@
"publisher": "llvm-vs-code-extensions",
"homepage": "https://clang.llvm.org/extra/clangd.html;,
"engines": {
-"vscode": "^1.18.0"
+"vscode": "^1.27.0"
},
"categories": [
"Programming Languages",
@@ -32,8 +32,8 @@
"test": "node ./node_modules/vscode/bin/test"
},
"dependencies": {
-"vscode-languageclient": "^4.0.0",
-"vscode-languageserver": "^4.0.0"
+"vscode-languageclient": "^5.1.0",
+"vscode-languageserver": "^5.1.0"
},
"devDependencies": {
"typescript": "^2.0.3",
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -40,6 +40,7 @@
InvalidRequest = -32600,
MethodNotFound = -32601,
InvalidParams = -32602,
+
InternalError = -32603,
ServerNotInitialized = -32002,
@@ -623,6 +624,38 @@
llvm::json::Value toJSON(const Command );
+/// Represents programming constructs like variables, classes, interfaces etc.
+/// that appear in a document. Document symbols can be hierarchical and they
+/// have two ranges: one that encloses its definition and one that points to its
+/// most interesting range, e.g. the range of an identifier.
+struct DocumentSymbol {
+ /// The name of this symbol.
+ std::string name;
+
+ /// More detail for this symbol, e.g the signature of a function.
+ std::string detail;
+
+ /// The kind of this symbol.
+ SymbolKind kind;
+
+ /// Indicates if this symbol is deprecated.
+ bool deprecated;
+
+ /// The range enclosing this symbol not including leading/trailing whitespace
+ /// but everything else like comments. This information is typically used to
+ /// determine if the clients cursor is inside the symbol to reveal in the
+ /// symbol in the UI.
+ Range range;
+
+ /// The range that should be selected and revealed when this symbol is being
+ /// picked, e.g the name of a function. Must be contained by the `range`.
+ Range selectionRange;
+
+ /// Children of this symbol, e.g. properties of a class.
+ std::vector children;
+};
+llvm::json::Value toJSON(const DocumentSymbol );
+
/// Represents information about programming constructs like variables, classes,
/// interfaces etc.
struct SymbolInformation {
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -18,6 +18,7 @@
#include "llvm/ADT/SmallString.h"
#include "llvm/Support/Format.h"
#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/raw_ostream.h"
@@ -448,6 +449,16 @@
return std::move(Cmd);
}
+llvm::json::Value toJSON(const DocumentSymbol ) {
+ return json::Object{{"name", S.name},
+ {"detail", S.detail},
+ {"kind", static_cast(S.kind)},
+ {"deprecated", S.deprecated},
+ {"children", json::Array{S.children}},
+ {"range", S.range},
+ {"selectionRange", S.selectionRange}};
+}
+
json::Value toJSON(const WorkspaceEdit ) {
if (!WE.changes)
return json::Object{};
Index: clangd/FindSymbols.h
===
--- clangd/FindSymbols.h
+++ clangd/FindSymbols.h
@@ -36,8 +36,7 @@
/// Retrieves the symbols contained in the "main file" section of an AST in the
/// same order
