[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle added a comment. In https://reviews.llvm.org/D44882#1077516, @sammccall wrote: > It makes sense, but @bkramer came up with some deep magic in > https://reviews.llvm.org/rL330754 so I think we're actually good now. Nice! Thanks @bkramer ! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
sammccall added a subscriber: bkramer. sammccall added a comment. In https://reviews.llvm.org/D44882#1076927, @malaperle wrote: > In https://reviews.llvm.org/D44882#1076864, @sammccall wrote: > > > So this fails if there's no standard library available without flags, which > > is the case in google's test environment to ensure hermeticity :-( > > > > In the short-term, we've disabled the test internally - did it trigger any > > buildbot failures? > > In the medium term we should probably find a better way to test this :( > > > No buildbot failures yet. But I suggest we just remove the test and add it > back when we collect more symbols, i.e. in main files. WDYT? It makes sense, but @bkramer came up with some deep magic in https://reviews.llvm.org/rL330754 so I think we're actually good now. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle added a comment. In https://reviews.llvm.org/D44882#1076864, @sammccall wrote: > So this fails if there's no standard library available without flags, which > is the case in google's test environment to ensure hermeticity :-( > > In the short-term, we've disabled the test internally - did it trigger any > buildbot failures? > In the medium term we should probably find a better way to test this :( No buildbot failures yet. But I suggest we just remove the test and add it back when we collect more symbols, i.e. in main files. WDYT? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
sammccall added a comment. In https://reviews.llvm.org/D44882#1066743, @malaperle wrote: > In https://reviews.llvm.org/D44882#1065632, @sammccall wrote: > > > In https://reviews.llvm.org/D44882#1065631, @malaperle wrote: > > > > > In https://reviews.llvm.org/D44882#1065622, @sammccall wrote: > > > > > > > Still LG, thanks! > > > > I'll look into the testing issue. > > > > > > > > > I thought about it after... I think it was because I was trying to test > > > with std::unordered_map (to prevent multiple results) which needs > > > std=c++11, I'll try with something else. > > > > > > Worth a shot, but don't count on it - for c++ clang switched to using > > std=c++11 by default a while ago. > > > I just tried "std::basic_ostringstream" and that works. Seems safer. So this fails if there's no standard library available without flags, which is the case in google's test environment to ensure hermeticity :-( In the short-term, we've disabled the test internally - did it trigger any buildbot failures? In the medium term we should probably find a better way to test this :( Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE330637: [clangd] Implementation of workspace/symbol
request (authored by malaperle, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D44882?vs=143220&id=143626#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
Files:
clangd/CMakeLists.txt
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/index/SymbolCollector.cpp
clangd/tool/ClangdMain.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
test/clangd/symbols.test
unittests/clangd/CMakeLists.txt
unittests/clangd/FindSymbolsTests.cpp
unittests/clangd/SyncAPI.cpp
unittests/clangd/SyncAPI.h
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- unittests/clangd/FindSymbolsTests.cpp
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -0,0 +1,247 @@
+//===-- FindSymbolsTests.cpp -*- C++ -*===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "Annotations.h"
+#include "ClangdServer.h"
+#include "FindSymbols.h"
+#include "SyncAPI.h"
+#include "TestFS.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+namespace {
+
+using ::testing::AllOf;
+using ::testing::AnyOf;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAre;
+
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+ void onDiagnosticsReady(PathRef File,
+ std::vector Diagnostics) override {}
+};
+
+// GMock helpers for matching SymbolInfos items.
+MATCHER_P(Named, Name, "") { return arg.name == Name; }
+MATCHER_P(InContainer, ContainerName, "") {
+ return arg.containerName == ContainerName;
+}
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+
+ClangdServer::Options optsForTests() {
+ auto ServerOpts = ClangdServer::optsForTest();
+ ServerOpts.BuildDynamicSymbolIndex = true;
+ return ServerOpts;
+}
+
+class WorkspaceSymbolsTest : public ::testing::Test {
+public:
+ WorkspaceSymbolsTest()
+ : Server(CDB, FSProvider, DiagConsumer, optsForTests()) {}
+
+protected:
+ MockFSProvider FSProvider;
+ MockCompilationDatabase CDB;
+ IgnoreDiagnostics DiagConsumer;
+ ClangdServer Server;
+ int Limit;
+
+ std::vector getSymbols(StringRef Query) {
+EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for preamble";
+auto SymbolInfos = runWorkspaceSymbols(Server, Query, Limit);
+EXPECT_TRUE(bool(SymbolInfos)) << "workspaceSymbols returned an error";
+return *SymbolInfos;
+ }
+
+ void addFile(StringRef FileName, StringRef Contents) {
+auto Path = testPath(FileName);
+FSProvider.Files[Path] = Contents;
+Server.addDocument(Path, Contents);
+ }
+};
+
+} // namespace
+
+TEST_F(WorkspaceSymbolsTest, NoMacro) {
+ addFile("foo.cpp", R"cpp(
+ #define MACRO X
+ )cpp");
+
+ // Macros are not in the index.
+ EXPECT_THAT(getSymbols("macro"), IsEmpty());
+}
+
+TEST_F(WorkspaceSymbolsTest, NoLocals) {
+ addFile("foo.cpp", R"cpp(
+ void test(int FirstParam, int SecondParam) {
+struct LocalClass {};
+int local_var;
+ })cpp");
+ EXPECT_THAT(getSymbols("l"), IsEmpty());
+ EXPECT_THAT(getSymbols("p"), IsEmpty());
+}
+
+TEST_F(WorkspaceSymbolsTest, Globals) {
+ addFile("foo.h", R"cpp(
+ int global_var;
+
+ int global_func();
+
+ struct GlobalStruct {};)cpp");
+ addFile("foo.cpp", R"cpp(
+ #include "foo.h"
+ )cpp");
+ EXPECT_THAT(getSymbols("global"),
+ UnorderedElementsAre(AllOf(Named("GlobalStruct"), InContainer(""),
+ WithKind(SymbolKind::Struct)),
+ AllOf(Named("global_func"), InContainer(""),
+ WithKind(SymbolKind::Function)),
+ AllOf(Named("global_var"), InContainer(""),
+ WithKind(SymbolKind::Variable;
+}
+
+TEST_F(WorkspaceSymbolsTest, Unnamed) {
+ addFile("foo.h", R"cpp(
+ struct {
+int InUnnamed;
+ } UnnamedStruct;)cpp");
+ addFile("foo.cpp", R"cpp(
+ #include "foo.h"
+ )cpp");
+ EXPECT_THAT(getSymbols("UnnamedStruct"),
+ ElementsAre(AllOf(Named("UnnamedStruct"),
+WithKind(SymbolKind::Variable;
+ EXPECT_THAT(getSymbols("InUnnamed"), IsEmpty());
+}
+
+TES
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle updated this revision to Diff 143220.
malaperle added a comment.
Remove more includes.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
Files:
clangd/CMakeLists.txt
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/index/SymbolCollector.cpp
clangd/tool/ClangdMain.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
test/clangd/symbols.test
unittests/clangd/CMakeLists.txt
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
@@ -41,6 +41,9 @@
std::string runDumpAST(ClangdServer &Server, PathRef File);
+llvm::Expected>
+runWorkspaceSymbols(ClangdServer &Server, StringRef Query, int Limit);
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -110,5 +110,12 @@
return std::move(*Result);
}
+llvm::Expected>
+runWorkspaceSymbols(ClangdServer &Server, StringRef Query, int Limit) {
+ llvm::Optional>> Result;
+ Server.workspaceSymbols(Query, Limit, capture(Result));
+ return std::move(*Result);
+}
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- /dev/null
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -0,0 +1,247 @@
+//===-- FindSymbolsTests.cpp -*- C++ -*===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "Annotations.h"
+#include "ClangdServer.h"
+#include "FindSymbols.h"
+#include "SyncAPI.h"
+#include "TestFS.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+namespace {
+
+using ::testing::AllOf;
+using ::testing::AnyOf;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAre;
+
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+ void onDiagnosticsReady(PathRef File,
+ std::vector Diagnostics) override {}
+};
+
+// GMock helpers for matching SymbolInfos items.
+MATCHER_P(Named, Name, "") { return arg.name == Name; }
+MATCHER_P(InContainer, ContainerName, "") {
+ return arg.containerName == ContainerName;
+}
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+
+ClangdServer::Options optsForTests() {
+ auto ServerOpts = ClangdServer::optsForTest();
+ ServerOpts.BuildDynamicSymbolIndex = true;
+ return ServerOpts;
+}
+
+class WorkspaceSymbolsTest : public ::testing::Test {
+public:
+ WorkspaceSymbolsTest()
+ : Server(CDB, FSProvider, DiagConsumer, optsForTests()) {}
+
+protected:
+ MockFSProvider FSProvider;
+ MockCompilationDatabase CDB;
+ IgnoreDiagnostics DiagConsumer;
+ ClangdServer Server;
+ int Limit;
+
+ std::vector getSymbols(StringRef Query) {
+EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for preamble";
+auto SymbolInfos = runWorkspaceSymbols(Server, Query, Limit);
+EXPECT_TRUE(bool(SymbolInfos)) << "workspaceSymbols returned an error";
+return *SymbolInfos;
+ }
+
+ void addFile(StringRef FileName, StringRef Contents) {
+auto Path = testPath(FileName);
+FSProvider.Files[Path] = Contents;
+Server.addDocument(Path, Contents);
+ }
+};
+
+} // namespace
+
+TEST_F(WorkspaceSymbolsTest, NoMacro) {
+ addFile("foo.cpp", R"cpp(
+ #define MACRO X
+ )cpp");
+
+ // Macros are not in the index.
+ EXPECT_THAT(getSymbols("macro"), IsEmpty());
+}
+
+TEST_F(WorkspaceSymbolsTest, NoLocals) {
+ addFile("foo.cpp", R"cpp(
+ void test(int FirstParam, int SecondParam) {
+struct LocalClass {};
+int local_var;
+ })cpp");
+ EXPECT_THAT(getSymbols("l"), IsEmpty());
+ EXPECT_THAT(getSymbols("p"), IsEmpty());
+}
+
+TEST_F(WorkspaceSymbolsTest, Globals) {
+ addFile("foo.h", R"cpp(
+ int global_var;
+
+ int global_func();
+
+ struct GlobalStruct {};)cpp");
+ addFile("foo.cpp", R"cpp(
+ #include "foo.h"
+ )cpp");
+ EXPECT_THAT(getSymbols("global"),
+ UnorderedElementsAre(AllOf(Named("GlobalStruct"), InContainer(""),
+ WithKind(SymbolKind::Struct)),
+ AllOf(Named("global_func"), InContainer(""),
+
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle added a comment. Sorry for the embarrassing clean-ups I forgot! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle updated this revision to Diff 143219.
malaperle marked 5 inline comments as done.
malaperle added a comment.
Address comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
Files:
clangd/CMakeLists.txt
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/index/SymbolCollector.cpp
clangd/tool/ClangdMain.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
test/clangd/symbols.test
unittests/clangd/CMakeLists.txt
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
@@ -41,6 +41,9 @@
std::string runDumpAST(ClangdServer &Server, PathRef File);
+llvm::Expected>
+runWorkspaceSymbols(ClangdServer &Server, StringRef Query, int Limit);
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -110,5 +110,12 @@
return std::move(*Result);
}
+llvm::Expected>
+runWorkspaceSymbols(ClangdServer &Server, StringRef Query, int Limit) {
+ llvm::Optional>> Result;
+ Server.workspaceSymbols(Query, Limit, capture(Result));
+ return std::move(*Result);
+}
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- /dev/null
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -0,0 +1,247 @@
+//===-- FindSymbolsTests.cpp -*- C++ -*===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "Annotations.h"
+#include "ClangdServer.h"
+#include "FindSymbols.h"
+#include "SyncAPI.h"
+#include "TestFS.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+namespace {
+
+using ::testing::AllOf;
+using ::testing::AnyOf;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAre;
+
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+ void onDiagnosticsReady(PathRef File,
+ std::vector Diagnostics) override {}
+};
+
+// GMock helpers for matching SymbolInfos items.
+MATCHER_P(Named, Name, "") { return arg.name == Name; }
+MATCHER_P(InContainer, ContainerName, "") {
+ return arg.containerName == ContainerName;
+}
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+
+ClangdServer::Options optsForTests() {
+ auto ServerOpts = ClangdServer::optsForTest();
+ ServerOpts.BuildDynamicSymbolIndex = true;
+ return ServerOpts;
+}
+
+class WorkspaceSymbolsTest : public ::testing::Test {
+public:
+ WorkspaceSymbolsTest()
+ : Server(CDB, FSProvider, DiagConsumer, optsForTests()) {}
+
+protected:
+ MockFSProvider FSProvider;
+ MockCompilationDatabase CDB;
+ IgnoreDiagnostics DiagConsumer;
+ ClangdServer Server;
+ int Limit;
+
+ std::vector getSymbols(StringRef Query) {
+EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for preamble";
+auto SymbolInfos = runWorkspaceSymbols(Server, Query, Limit);
+EXPECT_TRUE(bool(SymbolInfos)) << "workspaceSymbols returned an error";
+return *SymbolInfos;
+ }
+
+ void addFile(StringRef FileName, StringRef Contents) {
+auto Path = testPath(FileName);
+FSProvider.Files[Path] = Contents;
+Server.addDocument(Path, Contents);
+ }
+};
+
+} // namespace
+
+TEST_F(WorkspaceSymbolsTest, NoMacro) {
+ addFile("foo.cpp", R"cpp(
+ #define MACRO X
+ )cpp");
+
+ // Macros are not in the index.
+ EXPECT_THAT(getSymbols("macro"), IsEmpty());
+}
+
+TEST_F(WorkspaceSymbolsTest, NoLocals) {
+ addFile("foo.cpp", R"cpp(
+ void test(int FirstParam, int SecondParam) {
+struct LocalClass {};
+int local_var;
+ })cpp");
+ EXPECT_THAT(getSymbols("l"), IsEmpty());
+ EXPECT_THAT(getSymbols("p"), IsEmpty());
+}
+
+TEST_F(WorkspaceSymbolsTest, Globals) {
+ addFile("foo.h", R"cpp(
+ int global_var;
+
+ int global_func();
+
+ struct GlobalStruct {};)cpp");
+ addFile("foo.cpp", R"cpp(
+ #include "foo.h"
+ )cpp");
+ EXPECT_THAT(getSymbols("global"),
+ UnorderedElementsAre(AllOf(Named("GlobalStruct"), InContainer(""),
+ WithKind(SymbolKind::Struct)),
+ AllOf(Named("global_
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
hokein added inline comments.
Herald added a subscriber: jkorous.
Comment at: clangd/FindSymbols.cpp:117
+}
+auto Path = URI::resolve(*Uri);
+if (!Path) {
The current URI scheme (`file`, `test`) works fine without `HintPath` because
they don't use it, but for some custom URI schemes, they might rely on the
`HintPath`(e.g. the path of current main file) to resolve the absolute path
correctly. Maybe add a FIXME here?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
sammccall added a comment. Sorry, missed this. Just a few leftovers from removing the line/col conversion code. Then ship it! Comment at: clangd/ClangdServer.h:166 + // FIXME: Remove this parameter when the index has line/col. + const DraftStore &DS, + Callback> CB); DraftStore is now unused and can be dropped :-) (also the forward decl of it) Comment at: clangd/FindSymbols.cpp:11 + +#include "DraftStore.h" +#include "Logger.h" dead include Comment at: clangd/FindSymbols.h:13 +//===--===// +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_WORKSPACESYMBOL_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_WORKSPACESYMBOL_H nit: header guards no longer match filename Comment at: clangd/FindSymbols.h:24 +class SymbolIndex; +class DraftStore; + dead forward decl (and the VirtualFileSystem and IntrusiveRefCntPtr includes) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle added a comment. Any objections to the latest version? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle added a comment. In https://reviews.llvm.org/D44882#1066883, @hokein wrote: > Thanks, I have landed the patch today, you need to update your patch (I think > it is mostly about removing the code of reading-file stuff). I updated the patch using the new line/col from the index. This is great! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle updated this revision to Diff 142403.
malaperle marked 2 inline comments as done.
malaperle added a comment.
Address comments. Simplify with using line/col from index.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
Files:
clangd/CMakeLists.txt
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
clangd/index/SymbolCollector.cpp
clangd/tool/ClangdMain.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
test/clangd/symbols.test
unittests/clangd/CMakeLists.txt
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
@@ -41,6 +41,10 @@
std::string runDumpAST(ClangdServer &Server, PathRef File);
+llvm::Expected>
+runWorkspaceSymbols(ClangdServer &Server, StringRef Query, int Limit,
+const DraftStore &DS);
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -110,5 +110,13 @@
return std::move(*Result);
}
+llvm::Expected>
+runWorkspaceSymbols(ClangdServer &Server, StringRef Query, int Limit,
+const DraftStore &DS) {
+ llvm::Optional>> Result;
+ Server.workspaceSymbols(Query, Limit, DS, capture(Result));
+ return std::move(*Result);
+}
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- /dev/null
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -0,0 +1,249 @@
+//===-- FindSymbolsTests.cpp -*- C++ -*===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "Annotations.h"
+#include "ClangdServer.h"
+#include "DraftStore.h"
+#include "FindSymbols.h"
+#include "SyncAPI.h"
+#include "TestFS.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+namespace {
+
+using ::testing::AllOf;
+using ::testing::AnyOf;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAre;
+
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+ void onDiagnosticsReady(PathRef File,
+ std::vector Diagnostics) override {}
+};
+
+// GMock helpers for matching SymbolInfos items.
+MATCHER_P(Named, Name, "") { return arg.name == Name; }
+MATCHER_P(InContainer, ContainerName, "") {
+ return arg.containerName == ContainerName;
+}
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+
+ClangdServer::Options optsForTests() {
+ auto ServerOpts = ClangdServer::optsForTest();
+ ServerOpts.BuildDynamicSymbolIndex = true;
+ return ServerOpts;
+}
+
+class WorkspaceSymbolsTest : public ::testing::Test {
+public:
+ WorkspaceSymbolsTest()
+ : Server(CDB, FSProvider, DiagConsumer, optsForTests()) {}
+
+protected:
+ MockFSProvider FSProvider;
+ MockCompilationDatabase CDB;
+ IgnoreDiagnostics DiagConsumer;
+ ClangdServer Server;
+ int Limit;
+ DraftStore DS;
+
+ std::vector getSymbols(StringRef Query) {
+EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for preamble";
+auto SymbolInfos = runWorkspaceSymbols(Server, Query, Limit, DS);
+EXPECT_TRUE(bool(SymbolInfos)) << "workspaceSymbols returned an error";
+return *SymbolInfos;
+ }
+
+ void addFile(StringRef FileName, StringRef Contents) {
+auto Path = testPath(FileName);
+FSProvider.Files[Path] = Contents;
+Server.addDocument(Path, Contents);
+ }
+};
+
+} // namespace
+
+TEST_F(WorkspaceSymbolsTest, NoMacro) {
+ addFile("foo.cpp", R"cpp(
+ #define MACRO X
+ )cpp");
+
+ // Macros are not in the index.
+ EXPECT_THAT(getSymbols("macro"), IsEmpty());
+}
+
+TEST_F(WorkspaceSymbolsTest, NoLocals) {
+ addFile("foo.cpp", R"cpp(
+ void test(int FirstParam, int SecondParam) {
+struct LocalClass {};
+int local_var;
+ })cpp");
+ EXPECT_THAT(getSymbols("l"), IsEmpty());
+ EXPECT_THAT(getSymbols("p"), IsEmpty());
+}
+
+TEST_F(WorkspaceSymbolsTest, Globals) {
+ addFile("foo.h", R"cpp(
+ int global_var;
+
+ int global_func();
+
+ struct GlobalStruct {};)cpp");
+ addFile("foo.cpp", R"cpp(
+ #include "foo.h"
+ )cpp");
+ EXPECT_THAT(getSymbols("global"),
+
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
sammccall accepted this revision. sammccall added a comment. Still LG Comment at: clangd/ClangdLSPServer.cpp:113 + auto KindVal = static_cast(Kind); + if (KindVal >= SymbolKindMin && KindVal <= SymbolKindMax) +SupportedSymbolKinds.set(KindVal); malaperle wrote: > sammccall wrote: > > nit: the bounds checks at usage types, with explicit underlying type for > > the enum are inconsistent with what we do in other protocol enums, and seem > > to clutter the code. > > > > All else equal, I'd prefer to have an enum/enum class with implicit type, > > and not have values that are out of the enum's range. It's a simpler model > > that matches the code we have, and doesn't need tricky casts to > > SymKindUnderlyingType. If we want to support clients that send higher > > numbers than we know about, we could either: > > - overload fromJSON(vector)& > > - just express the capabilities as vector and convert them to the > > enum (and check bounds) at this point. > > WDYT? > I think it's better to keep vector in Protocol.h, it is clearer > and more in line with the protocol. I overloaded fromJSON, which simplifies > the code here, but it still needs a static_cast to size_t for the > bitset.set(). Other places there still needs a static_cast, like in > defaultSymbolKinds for the loop, I can static_cast to size_t everywhere (or > int?) but having SymKindUnderlyingType seems more correct. I changed it to > size_t, let me know if it was something like that you had in mind. LG, though I've commented in one place where the cast seems necessary. Personally I usually use int here, but size_t is good too. The exact type doesn't matter as long as it covers all values of the enum. Comment at: clangd/ClangdLSPServer.cpp:274 + Sym.kind = adjustKindToCapability(Sym.kind, SupportedSymbolKinds); + assert(static_cast(Sym.kind) >= SymbolKindMin && + static_cast(Sym.kind) < SupportedSymbolKinds.size() && you no longer need the min/max asserts and their casts, because we don't allow any values of type SymbolKind that don't have one of the enum values (i.e. we're just echoing the type system here) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
hokein added a comment. In https://reviews.llvm.org/D44882#1065932, @malaperle wrote: > In https://reviews.llvm.org/D44882#1065787, @hokein wrote: > > > @malaperle, what's your plan of this patch? Are you going to land it before > > https://reviews.llvm.org/D45513? With the Line&Column info in the index, > > this patch could be simplified. > > > I'll address the last comment and wait for review. Probably at least another > day of delay. So if that ends up after https://reviews.llvm.org/D45513 then > I'll update it to simplify. Sounds good? Thanks, I have landed the patch today, you need to update your patch (I think it is mostly about removing the code of reading-file stuff). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle marked an inline comment as done. malaperle added inline comments. Comment at: clangd/ClangdLSPServer.cpp:113 + auto KindVal = static_cast(Kind); + if (KindVal >= SymbolKindMin && KindVal <= SymbolKindMax) +SupportedSymbolKinds.set(KindVal); sammccall wrote: > nit: the bounds checks at usage types, with explicit underlying type for the > enum are inconsistent with what we do in other protocol enums, and seem to > clutter the code. > > All else equal, I'd prefer to have an enum/enum class with implicit type, and > not have values that are out of the enum's range. It's a simpler model that > matches the code we have, and doesn't need tricky casts to > SymKindUnderlyingType. If we want to support clients that send higher numbers > than we know about, we could either: > - overload fromJSON(vector)& > - just express the capabilities as vector and convert them to the enum > (and check bounds) at this point. > WDYT? I think it's better to keep vector in Protocol.h, it is clearer and more in line with the protocol. I overloaded fromJSON, which simplifies the code here, but it still needs a static_cast to size_t for the bitset.set(). Other places there still needs a static_cast, like in defaultSymbolKinds for the loop, I can static_cast to size_t everywhere (or int?) but having SymKindUnderlyingType seems more correct. I changed it to size_t, let me know if it was something like that you had in mind. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle updated this revision to Diff 142328.
malaperle marked an inline comment as done.
malaperle added a comment.
Address comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
Files:
clangd/CMakeLists.txt
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
clangd/index/SymbolCollector.cpp
clangd/tool/ClangdMain.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
test/clangd/symbols.test
unittests/clangd/CMakeLists.txt
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
@@ -41,6 +41,10 @@
std::string runDumpAST(ClangdServer &Server, PathRef File);
+llvm::Expected>
+runWorkspaceSymbols(ClangdServer &Server, StringRef Query, int Limit,
+const DraftStore &DS);
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -110,5 +110,13 @@
return std::move(*Result);
}
+llvm::Expected>
+runWorkspaceSymbols(ClangdServer &Server, StringRef Query, int Limit,
+const DraftStore &DS) {
+ llvm::Optional>> Result;
+ Server.workspaceSymbols(Query, Limit, DS, capture(Result));
+ return std::move(*Result);
+}
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- /dev/null
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -0,0 +1,249 @@
+//===-- FindSymbolsTests.cpp -*- C++ -*===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "Annotations.h"
+#include "ClangdServer.h"
+#include "DraftStore.h"
+#include "FindSymbols.h"
+#include "SyncAPI.h"
+#include "TestFS.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+namespace {
+
+using ::testing::AllOf;
+using ::testing::AnyOf;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAre;
+
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+ void onDiagnosticsReady(PathRef File,
+ std::vector Diagnostics) override {}
+};
+
+// GMock helpers for matching SymbolInfos items.
+MATCHER_P(Named, Name, "") { return arg.name == Name; }
+MATCHER_P(InContainer, ContainerName, "") {
+ return arg.containerName == ContainerName;
+}
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+
+ClangdServer::Options optsForTests() {
+ auto ServerOpts = ClangdServer::optsForTest();
+ ServerOpts.BuildDynamicSymbolIndex = true;
+ return ServerOpts;
+}
+
+class WorkspaceSymbolsTest : public ::testing::Test {
+public:
+ WorkspaceSymbolsTest()
+ : Server(CDB, FSProvider, DiagConsumer, optsForTests()) {}
+
+protected:
+ MockFSProvider FSProvider;
+ MockCompilationDatabase CDB;
+ IgnoreDiagnostics DiagConsumer;
+ ClangdServer Server;
+ int Limit;
+ DraftStore DS;
+
+ std::vector getSymbols(StringRef Query) {
+EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for preamble";
+auto SymbolInfos = runWorkspaceSymbols(Server, Query, Limit, DS);
+EXPECT_TRUE(bool(SymbolInfos)) << "workspaceSymbols returned an error";
+return *SymbolInfos;
+ }
+
+ void addFile(StringRef FileName, StringRef Contents) {
+auto Path = testPath(FileName);
+FSProvider.Files[Path] = Contents;
+Server.addDocument(Path, Contents);
+ }
+};
+
+} // namespace
+
+TEST_F(WorkspaceSymbolsTest, NoMacro) {
+ addFile("foo.cpp", R"cpp(
+ #define MACRO X
+ )cpp");
+
+ // Macros are not in the index.
+ EXPECT_THAT(getSymbols("macro"), IsEmpty());
+}
+
+TEST_F(WorkspaceSymbolsTest, NoLocals) {
+ addFile("foo.cpp", R"cpp(
+ void test(int FirstParam, int SecondParam) {
+struct LocalClass {};
+int local_var;
+ })cpp");
+ EXPECT_THAT(getSymbols("l"), IsEmpty());
+ EXPECT_THAT(getSymbols("p"), IsEmpty());
+}
+
+TEST_F(WorkspaceSymbolsTest, Globals) {
+ addFile("foo.h", R"cpp(
+ int global_var;
+
+ int global_func();
+
+ struct GlobalStruct {};)cpp");
+ addFile("foo.cpp", R"cpp(
+ #include "foo.h"
+ )cpp");
+ EXPECT_THAT(getSymbols("global"),
+ UnorderedElementsAre(AllOf(Named("Globa
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle added a comment. In https://reviews.llvm.org/D44882#1065632, @sammccall wrote: > In https://reviews.llvm.org/D44882#1065631, @malaperle wrote: > > > In https://reviews.llvm.org/D44882#1065622, @sammccall wrote: > > > > > Still LG, thanks! > > > I'll look into the testing issue. > > > > > > I thought about it after... I think it was because I was trying to test > > with std::unordered_map (to prevent multiple results) which needs > > std=c++11, I'll try with something else. > > > Worth a shot, but don't count on it - for c++ clang switched to using > std=c++11 by default a while ago. I just tried "std::basic_ostringstream" and that works. Seems safer. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle added a comment. In https://reviews.llvm.org/D44882#1065787, @hokein wrote: > @malaperle, what's your plan of this patch? Are you going to land it before > https://reviews.llvm.org/D45513? With the Line&Column info in the index, this > patch could be simplified. I'll address the last comment and wait for review. Probably at least another day of delay. So if that ends up after https://reviews.llvm.org/D45513 then I'll update it to simplify. Sounds good? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
hokein added a comment. @malaperle, what's your plan of this patch? Are you going to land it before https://reviews.llvm.org/D45513? With the Line&Column info in the index, this patch could be simplified. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
sammccall added a comment. In https://reviews.llvm.org/D44882#1065631, @malaperle wrote: > In https://reviews.llvm.org/D44882#1065622, @sammccall wrote: > > > Still LG, thanks! > > I'll look into the testing issue. > > > I thought about it after... I think it was because I was trying to test with > std::unordered_map (to prevent multiple results) which needs std=c++11, I'll > try with something else. Worth a shot, but don't count on it - for c++ clang switched to using std=c++11 by default a while ago. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle added a comment. In https://reviews.llvm.org/D44882#1065622, @sammccall wrote: > Still LG, thanks! > I'll look into the testing issue. I thought about it after... I think it was because I was trying to test with std::unordered_map (to prevent multiple results) which needs std=c++11, I'll try with something else. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
sammccall added a comment. Still LG, thanks! I'll look into the testing issue. Comment at: clangd/ClangdLSPServer.cpp:113 + auto KindVal = static_cast(Kind); + if (KindVal >= SymbolKindMin && KindVal <= SymbolKindMax) +SupportedSymbolKinds.set(KindVal); nit: the bounds checks at usage types, with explicit underlying type for the enum are inconsistent with what we do in other protocol enums, and seem to clutter the code. All else equal, I'd prefer to have an enum/enum class with implicit type, and not have values that are out of the enum's range. It's a simpler model that matches the code we have, and doesn't need tricky casts to SymKindUnderlyingType. If we want to support clients that send higher numbers than we know about, we could either: - overload fromJSON(vector)& - just express the capabilities as vector and convert them to the enum (and check bounds) at this point. WDYT? Comment at: clangd/FindSymbols.h:26 + +llvm::Expected> getWorkspaceSymbols( +llvm::StringRef Query, int Limit, const SymbolIndex *const Index, Would be nice to have a comment describing: query syntax, limit=0 special behavior. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle added a comment.
In https://reviews.llvm.org/D44882#1064196, @sammccall wrote:
> (BTW @hokein has https://reviews.llvm.org/D45513 to add row/col to the index,
> which will allow all the file-reading stuff to be cleaned up, but no need to
> wait on that since it's all working now).
Looking forward to it!
Comment at: clangd/ClangdLSPServer.cpp:101
+ // All clients should support those.
+ for (SymKindUnderlyingType I = SymbolKindMin;
+ I <= static_cast(SymbolKind::Array); ++I)
sammccall wrote:
> malaperle wrote:
> > I'd like to add some test to exercise the changes in ClangdLSPServer.cpp
> > and Protocol.cpp but it's not straightforward to do nicely. Using a "lit"
> > test, I cannot have a header and since symbols in the main file are not
> > collected then it's not useful. If I make a gtest, I have to feed it the
> > lsp server with stdin and check the stdout which is a bit messy, but doable.
> Yeah, we don't have a good way to test ClangdLSPServer :-( I would like to
> fix that, but I don't know of any easy fixes.
>
> This is kind of gross, but do standard library includes work from our lit
> tests? You could `#include ` and then test using some symbols you know
> are there...
>
It doesn't seem to work unfortunately. I'm not sure why yet. Either way, I
think I would add this test as a separate patch if we are to add a standard
library include because I'm a bit nervous it will break and we will have to
revert it :)
Comment at: clangd/ClangdLSPServer.cpp:103
+ I <= static_cast(SymbolKind::Array); ++I)
+SupportedSymbolKinds.set(I);
+
sammccall wrote:
> I'd like to be slightly less hostile than this to (broken) clients that fail
> to call initialize.
> As the patch stands they'll get an empty SupportedSymbolKinds, and we'll
> crash if they call documentSymbols.
>
> Instead I'd suggest pulling out defaultSymbolKinds() and initializing to that
> in the constructor, and then overriding with either `SpecifiedSymbolKinds` or
> `SpecifiedSymbolKinds | defaultSymbolKinds()` here.
Good catch. I initialized SupportedSymbolKinds with defaultSymbolKinds() and
let this loop add additional symbol kinds.
Comment at: clangd/ClangdServer.h:164
+ void workspaceSymbols(StringRef Query,
+const clangd::WorkspaceSymbolOptions &Opts,
+const DraftStore &DS,
sammccall wrote:
> I think it would probably be more consistent with other functions to just
> take `int limit` here. I'm not sure CodeCompletion is an example we want to
> emulate.
>
> ClangdLSPServer might even just grab it from the code completion options,
> since that's the behavior we actually want.
>
> Up to you, though.
I think it makes sense. We can reintroduce an option struct when there is more
than one thing in it.
Comment at: clangd/Protocol.cpp:209
+ default:
+llvm_unreachable("Unexpected symbol kind");
+ }
sammccall wrote:
> This doesn't actually seem unreachable (or won't stay that way), maybe log
> and return `Null` or something like that? (Wow, there's really no catch-all
> option for this mandatory enum...)
`Null` Is not in LSP 1.0 and 2.0 that's why I had put `Variable` at the
beginning. Maybe `String` is better. Not sure what you think is best.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle updated this revision to Diff 142112.
malaperle marked 2 inline comments as done.
malaperle added a comment.
Address comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
Files:
clangd/CMakeLists.txt
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
clangd/index/SymbolCollector.cpp
clangd/tool/ClangdMain.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
unittests/clangd/CMakeLists.txt
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
@@ -41,6 +41,10 @@
std::string runDumpAST(ClangdServer &Server, PathRef File);
+llvm::Expected>
+runWorkspaceSymbols(ClangdServer &Server, StringRef Query, int Limit,
+const DraftStore &DS);
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -110,5 +110,13 @@
return std::move(*Result);
}
+llvm::Expected>
+runWorkspaceSymbols(ClangdServer &Server, StringRef Query, int Limit,
+const DraftStore &DS) {
+ llvm::Optional>> Result;
+ Server.workspaceSymbols(Query, Limit, DS, capture(Result));
+ return std::move(*Result);
+}
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- /dev/null
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -0,0 +1,249 @@
+//===-- FindSymbolsTests.cpp -*- C++ -*===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "Annotations.h"
+#include "ClangdServer.h"
+#include "DraftStore.h"
+#include "FindSymbols.h"
+#include "SyncAPI.h"
+#include "TestFS.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+namespace {
+
+using ::testing::AllOf;
+using ::testing::AnyOf;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAre;
+
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+ void onDiagnosticsReady(PathRef File,
+ std::vector Diagnostics) override {}
+};
+
+// GMock helpers for matching SymbolInfos items.
+MATCHER_P(Named, Name, "") { return arg.name == Name; }
+MATCHER_P(InContainer, ContainerName, "") {
+ return arg.containerName == ContainerName;
+}
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+
+ClangdServer::Options optsForTests() {
+ auto ServerOpts = ClangdServer::optsForTest();
+ ServerOpts.BuildDynamicSymbolIndex = true;
+ return ServerOpts;
+}
+
+class WorkspaceSymbolsTest : public ::testing::Test {
+public:
+ WorkspaceSymbolsTest()
+ : Server(CDB, FSProvider, DiagConsumer, optsForTests()) {}
+
+protected:
+ MockFSProvider FSProvider;
+ MockCompilationDatabase CDB;
+ IgnoreDiagnostics DiagConsumer;
+ ClangdServer Server;
+ int Limit;
+ DraftStore DS;
+
+ std::vector getSymbols(StringRef Query) {
+EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for preamble";
+auto SymbolInfos = runWorkspaceSymbols(Server, Query, Limit, DS);
+EXPECT_TRUE(bool(SymbolInfos)) << "workspaceSymbols returned an error";
+return *SymbolInfos;
+ }
+
+ void addFile(StringRef FileName, StringRef Contents) {
+auto Path = testPath(FileName);
+FSProvider.Files[Path] = Contents;
+Server.addDocument(Path, Contents);
+ }
+};
+
+} // namespace
+
+TEST_F(WorkspaceSymbolsTest, NoMacro) {
+ addFile("foo.cpp", R"cpp(
+ #define MACRO X
+ )cpp");
+
+ // Macros are not in the index.
+ EXPECT_THAT(getSymbols("macro"), IsEmpty());
+}
+
+TEST_F(WorkspaceSymbolsTest, NoLocals) {
+ addFile("foo.cpp", R"cpp(
+ void test(int FirstParam, int SecondParam) {
+struct LocalClass {};
+int local_var;
+ })cpp");
+ EXPECT_THAT(getSymbols("l"), IsEmpty());
+ EXPECT_THAT(getSymbols("p"), IsEmpty());
+}
+
+TEST_F(WorkspaceSymbolsTest, Globals) {
+ addFile("foo.h", R"cpp(
+ int global_var;
+
+ int global_func();
+
+ struct GlobalStruct {};)cpp");
+ addFile("foo.cpp", R"cpp(
+ #include "foo.h"
+ )cpp");
+ EXPECT_THAT(getSymbols("global"),
+ UnorderedElementsAre(AllOf(Named("GlobalStruct"), InContainer(""),
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
sammccall added a comment. (BTW @hokein has https://reviews.llvm.org/D45513 to add row/col to the index, which will allow all the file-reading stuff to be cleaned up, but no need to wait on that since it's all working now). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Sorry about the delay Marc-André - I got stuck on "how to test ClangdLSPServer"
and then distracted!
But this looks great.
Comment at: clangd/ClangdLSPServer.cpp:101
+ // All clients should support those.
+ for (SymKindUnderlyingType I = SymbolKindMin;
+ I <= static_cast(SymbolKind::Array); ++I)
malaperle wrote:
> I'd like to add some test to exercise the changes in ClangdLSPServer.cpp and
> Protocol.cpp but it's not straightforward to do nicely. Using a "lit" test, I
> cannot have a header and since symbols in the main file are not collected
> then it's not useful. If I make a gtest, I have to feed it the lsp server
> with stdin and check the stdout which is a bit messy, but doable.
Yeah, we don't have a good way to test ClangdLSPServer :-( I would like to fix
that, but I don't know of any easy fixes.
This is kind of gross, but do standard library includes work from our lit
tests? You could `#include ` and then test using some symbols you know are
there...
Comment at: clangd/ClangdLSPServer.cpp:103
+ I <= static_cast(SymbolKind::Array); ++I)
+SupportedSymbolKinds.set(I);
+
I'd like to be slightly less hostile than this to (broken) clients that fail to
call initialize.
As the patch stands they'll get an empty SupportedSymbolKinds, and we'll crash
if they call documentSymbols.
Instead I'd suggest pulling out defaultSymbolKinds() and initializing to that
in the constructor, and then overriding with either `SpecifiedSymbolKinds` or
`SpecifiedSymbolKinds | defaultSymbolKinds()` here.
Comment at: clangd/ClangdServer.h:164
+ void workspaceSymbols(StringRef Query,
+const clangd::WorkspaceSymbolOptions &Opts,
+const DraftStore &DS,
I think it would probably be more consistent with other functions to just take
`int limit` here. I'm not sure CodeCompletion is an example we want to emulate.
ClangdLSPServer might even just grab it from the code completion options, since
that's the behavior we actually want.
Up to you, though.
Comment at: clangd/ClangdServer.h:165
+const clangd::WorkspaceSymbolOptions &Opts,
+const DraftStore &DS,
+Callback> CB);
maybe a short FIXME here too e.g. "remove param when the index has line/col"
Comment at: clangd/FindSymbols.cpp:42
+
+ if (!supportedSymbolKinds) {
+// Provide some sensible default when all fails.
malaperle wrote:
> sammccall wrote:
> > This code shouldn't need to handle this case. The LSP specifies the default
> > set of supported types if it's not provided, so ClangdLSPServer should
> > enure we always have a valid set
> My thought is that if we start dealing with one of those:
> ```
> Object = 19,
> Key = 20,
> Null = 21,
> Event = 24,
> Operator = 25,
> TypeParameter = 26
> ```
> and it's an older client that only supports up to "Array = 18", then we can
> provide a fallback, otherwise the client my not handle the unknown kind
> gracefully. But we can add this later if necessary I guess.
Ah yes I agree, fallbacks are good. I just meant that ClangdLSPServer should
ensure that we actually have the bitset populated with the right values. I
think it looks good now.
Comment at: clangd/Protocol.cpp:209
+ default:
+llvm_unreachable("Unexpected symbol kind");
+ }
This doesn't actually seem unreachable (or won't stay that way), maybe log and
return `Null` or something like that? (Wow, there's really no catch-all option
for this mandatory enum...)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle added inline comments. Comment at: clangd/ClangdLSPServer.cpp:101 + // All clients should support those. + for (SymKindUnderlyingType I = SymbolKindMin; + I <= static_cast(SymbolKind::Array); ++I) I'd like to add some test to exercise the changes in ClangdLSPServer.cpp and Protocol.cpp but it's not straightforward to do nicely. Using a "lit" test, I cannot have a header and since symbols in the main file are not collected then it's not useful. If I make a gtest, I have to feed it the lsp server with stdin and check the stdout which is a bit messy, but doable. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle updated this revision to Diff 141028.
malaperle marked 27 inline comments as done.
malaperle added a comment.
Address comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
Files:
clangd/CMakeLists.txt
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
clangd/index/SymbolCollector.cpp
clangd/tool/ClangdMain.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
unittests/clangd/CMakeLists.txt
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
@@ -41,6 +41,10 @@
std::string runDumpAST(ClangdServer &Server, PathRef File);
+llvm::Expected>
+runWorkspaceSymbols(ClangdServer &Server, StringRef Query,
+const WorkspaceSymbolOptions &Opts, const DraftStore &DS);
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -110,5 +110,13 @@
return std::move(*Result);
}
+llvm::Expected>
+runWorkspaceSymbols(ClangdServer &Server, StringRef Query,
+const WorkspaceSymbolOptions &Opts, const DraftStore &DS) {
+ llvm::Optional>> Result;
+ Server.workspaceSymbols(Query, Opts, DS, capture(Result));
+ return std::move(*Result);
+}
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- /dev/null
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -0,0 +1,249 @@
+//===-- FindSymbolsTests.cpp -*- C++ -*===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "Annotations.h"
+#include "ClangdServer.h"
+#include "DraftStore.h"
+#include "FindSymbols.h"
+#include "SyncAPI.h"
+#include "TestFS.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+namespace {
+
+using ::testing::AllOf;
+using ::testing::AnyOf;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAre;
+
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+ void onDiagnosticsReady(PathRef File,
+ std::vector Diagnostics) override {}
+};
+
+// GMock helpers for matching SymbolInfos items.
+MATCHER_P(Named, Name, "") { return arg.name == Name; }
+MATCHER_P(InContainer, ContainerName, "") {
+ return arg.containerName == ContainerName;
+}
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+
+ClangdServer::Options optsForTests() {
+ auto ServerOpts = ClangdServer::optsForTest();
+ ServerOpts.BuildDynamicSymbolIndex = true;
+ return ServerOpts;
+}
+
+class WorkspaceSymbolsTest : public ::testing::Test {
+public:
+ WorkspaceSymbolsTest()
+ : Server(CDB, FSProvider, DiagConsumer, optsForTests()) {}
+
+protected:
+ MockFSProvider FSProvider;
+ MockCompilationDatabase CDB;
+ IgnoreDiagnostics DiagConsumer;
+ ClangdServer Server;
+ WorkspaceSymbolOptions Opts;
+ DraftStore DS;
+
+ std::vector getSymbols(StringRef Query) {
+EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for preamble";
+auto SymbolInfos = runWorkspaceSymbols(Server, Query, Opts, DS);
+EXPECT_TRUE(bool(SymbolInfos)) << "workspaceSymbols returned an error";
+return *SymbolInfos;
+ }
+
+ void addFile(StringRef FileName, StringRef Contents) {
+auto Path = testPath(FileName);
+FSProvider.Files[Path] = Contents;
+Server.addDocument(Path, Contents);
+ }
+};
+
+} // namespace
+
+TEST_F(WorkspaceSymbolsTest, NoMacro) {
+ addFile("foo.cpp", R"cpp(
+ #define MACRO X
+ )cpp");
+
+ // Macros are not in the index.
+ EXPECT_THAT(getSymbols("macro"), IsEmpty());
+}
+
+TEST_F(WorkspaceSymbolsTest, NoLocals) {
+ addFile("foo.cpp", R"cpp(
+ void test(int FirstParam, int SecondParam) {
+struct LocalClass {};
+int local_var;
+ })cpp");
+ EXPECT_THAT(getSymbols("l"), IsEmpty());
+ EXPECT_THAT(getSymbols("p"), IsEmpty());
+}
+
+TEST_F(WorkspaceSymbolsTest, Globals) {
+ addFile("foo.h", R"cpp(
+ int global_var;
+
+ int global_func();
+
+ struct GlobalStruct {};)cpp");
+ addFile("foo.cpp", R"cpp(
+ #include "foo.h"
+ )cpp");
+ EXPECT_THAT(getSymbols("global"),
+
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:99
Params.capabilities.textDocument.completion.completionItem.snippetSupport;
+ if (Params.capabilities.workspace && Params.capabilities.workspace->symbol &&
+ Params.capabilities.workspace->symbol->symbolKind)
sammccall wrote:
> malaperle wrote:
> > sammccall wrote:
> > > This is the analogue to the one on `completion` that we currently ignore,
> > > and one on `symbol` corresponding to the `documentSymbol` call which
> > > isn't implemented yet.
> > >
> > > I think the value of the extended types is low and it might be worth
> > > leaving out of the first version to keep complexity down.
> > > (If we really want it, the supporting code (mapping tables) should
> > > probably be in a place where it can be reused by `documentSymbol` and can
> > > live next to the equivalent for `completion`...)
> > I think having Struct and EnumMember is nice and for sure once Macros is
> > there we will want to use it. So would it be OK to move to FindSymbols.cpp
> > (the new common file for document/symbol and workspace/symbol)?
> > I think having Struct and EnumMember is nice and for sure once Macros is
> > there we will want to use it.
> OK, fair enough.
>
> I think what bothers me about this option is how deep it goes - this isn't
> really a request option in any useful sense.
> I'd suggest the "C++ layer" i.e. ClangdServer should always return
> full-fidelity results, and the "LSP layer" should deal with folding them.
>
> In which case adjustKindToCapability should live somewhere like protocol.h
> and get called from this file - WDYT?
Sounds good, I moved it.
Comment at: clangd/ClangdLSPServer.cpp:258
+if (!Items)
+ return replyError(ErrorCode::InvalidParams,
+llvm::toString(Items.takeError()));
hokein wrote:
> `InvalidParams` doesn't match all cases where internal errors occur. Maybe
> use `ErrorCode::InternalError`?
Sounds good.
Comment at: clangd/FindSymbols.cpp:28
+ // All clients should support those.
+ if (Kind >= SymbolKind::File && Kind <= SymbolKind::Array)
+return Kind;
sammccall wrote:
> Can we rather have this condition checked when the client sets the supported
> symbols, and remove this special knowledge here?
"supportedSymbolKinds" is now populated with those base symbol kinds at
initialization.
Comment at: clangd/FindSymbols.cpp:42
+
+ if (!supportedSymbolKinds) {
+// Provide some sensible default when all fails.
sammccall wrote:
> This code shouldn't need to handle this case. The LSP specifies the default
> set of supported types if it's not provided, so ClangdLSPServer should enure
> we always have a valid set
My thought is that if we start dealing with one of those:
```
Object = 19,
Key = 20,
Null = 21,
Event = 24,
Operator = 25,
TypeParameter = 26
```
and it's an older client that only supports up to "Array = 18", then we can
provide a fallback, otherwise the client my not handle the unknown kind
gracefully. But we can add this later if necessary I guess.
Comment at: clangd/FindSymbols.cpp:114
+ std::vector Result;
+ if (Query.empty() || !Index)
+return Result;
sammccall wrote:
> why Query.empty()?
> In practice maybe showing results before you type is bad UX, but that seems
> up to the editor to decide.
vscode sends an empty query and it also doesn't show all symbols in Typescript
for example, so that's why I thought that would be best.
Comment at: clangd/FindSymbols.cpp:130
+
+ // Global scope is represented by "" in FuzzyFind.
+ if (Names.first.startswith("::"))
sammccall wrote:
> sammccall wrote:
> > nit: this is `Names.first.consume_front("::")`
> > Might be clearer as "FuzzyFind doesn't want leading :: qualifier" to avoid
> > implication, that global namespace is special, particularly because...
> actually the global namespace *is* special :-)
>
> IMO If you type `Foo`, you should get results from any namespace, but if you
> type `::Foo` you should only get results from the global namespace.
>
> So something like:
> ```
> if (Names.first.consume_front("::") || !Names.first.empty())
> Req.Scopes = {Names.first};
> ```
> though more explicit handling of the cases may be clearer
Sounds good. I think this works well.
Comment at: clangd/FindSymbols.cpp:141
+ Index->fuzzyFind(Req, [&TempSM, &Result, &Opts](const Symbol &Sym) {
+auto &CD = Sym.Definition ? Sym.Definition : Sym.CanonicalDeclaration;
+auto Uri = URI::parse(CD.FileURI);
sammccall wrote:
> Ugh, I never know what the right thing is here, decl/def split is a mess, and
> there's lots of different workflows.
>
> I don't have any problem with the choice made here (personally I'd probably
> prefe
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
hokein added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:258
+if (!Items)
+ return replyError(ErrorCode::InvalidParams,
+llvm::toString(Items.takeError()));
`InvalidParams` doesn't match all cases where internal errors occur. Maybe use
`ErrorCode::InternalError`?
Comment at: clangd/FindSymbols.cpp:145
+if (!Uri) {
+ log(llvm::formatv(
+ "Workspace symbol: Could not parse URI '{0}' for symbol '{1}'.",
I think we may want to report the error to client instead of just logging them.
Comment at: clangd/SourceCode.cpp:143
+/// From "a::b::c", return {"a::b::", "c"}. Scope is empty if there's no
+/// qualifier.
+std::pair
nit: this comment is duplicated with the one in header.
Comment at: clangd/SourceCode.h:59
+/// Turn a pair of offsets into a Location.
+llvm::Optional
+offsetRangeToLocation(const DraftStore &DS, SourceManager &SourceMgr,
We should use `llvm::Expected`?
The function needs a comment documenting its behavior, especially on the
unsaved file content.
Comment at: clangd/SourceCode.h:61
+offsetRangeToLocation(const DraftStore &DS, SourceManager &SourceMgr,
+ StringRef File, size_t OffsetStart, size_t OffsetEnd);
+
nit: name `FilePath` or `FileName`, `File` seems to be a bit confusing, does it
mean `FileContent` or `FileName`?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle marked an inline comment as done. malaperle added inline comments. Comment at: clangd/FindSymbols.cpp:31 + + if (supportedSymbolKinds && + std::find(supportedSymbolKinds->begin(), supportedSymbolKinds->end(), MaskRay wrote: > MaskRay wrote: > > malaperle wrote: > > > MaskRay wrote: > > > > This std::find loop adds some overhead to each candidate... In my > > > > experience the client usually doesn't care about the returned symbol > > > > kinds, they are used to give a category name. You can always patch the > > > > upstream to add missing categories. > > > > > > > > This is one instance where LSP is over specified. nvm I don't have > > > > strong opinion here > > > I have a client that throws an exception when the symbolkind is not known > > > and the whole request fails, so I think it's worth checking. But if it's > > > too slow I can look at making it faster. Unfortunately, I cannot patch > > > any upstream project :) > > https://github.com/gluon-lang/languageserver-types/blob/master/src/lib.rs#L2016 > > > > LanguageClient-neovim returns empty candidate list if one of the candidates > > has unknown SymbolKind. Apparently they should be more tolerant and there > > is an issue tracking it. > If it was not an internal confidential client, I would like to know its name, > unless the confidentiality includes the existence of the client. Sorry, I should have explained a bit more my thought. I see that an older-ish version of Monaco is not handling unknown symbols kinds well. I don't really know if it's our integration with Monaco or Monaco itself (the IDE is Theia). But my thought is that there are clients out there that follow the protocol "correctly" by choosing not to handle those unknown symbol kinds gracefully. I am not that concerned about a single client in particular, just general support of clients out there. As a server, I think it makes sense to support clients that follow the protocol, especially since 1.x and 2.x are not that old. I don't think it's the crux of the complexity of this patch. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
MaskRay added inline comments. Comment at: clangd/FindSymbols.cpp:31 + + if (supportedSymbolKinds && + std::find(supportedSymbolKinds->begin(), supportedSymbolKinds->end(), MaskRay wrote: > malaperle wrote: > > MaskRay wrote: > > > This std::find loop adds some overhead to each candidate... In my > > > experience the client usually doesn't care about the returned symbol > > > kinds, they are used to give a category name. You can always patch the > > > upstream to add missing categories. > > > > > > This is one instance where LSP is over specified. nvm I don't have strong > > > opinion here > > I have a client that throws an exception when the symbolkind is not known > > and the whole request fails, so I think it's worth checking. But if it's > > too slow I can look at making it faster. Unfortunately, I cannot patch any > > upstream project :) > https://github.com/gluon-lang/languageserver-types/blob/master/src/lib.rs#L2016 > > LanguageClient-neovim returns empty candidate list if one of the candidates > has unknown SymbolKind. Apparently they should be more tolerant and there is > an issue tracking it. If it was not an internal confidential client, I would like to know its name, unless the confidentiality includes the existence of the client. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
MaskRay added inline comments. Comment at: clangd/FindSymbols.cpp:31 + + if (supportedSymbolKinds && + std::find(supportedSymbolKinds->begin(), supportedSymbolKinds->end(), malaperle wrote: > MaskRay wrote: > > This std::find loop adds some overhead to each candidate... In my > > experience the client usually doesn't care about the returned symbol kinds, > > they are used to give a category name. You can always patch the upstream to > > add missing categories. > > > > This is one instance where LSP is over specified. nvm I don't have strong > > opinion here > I have a client that throws an exception when the symbolkind is not known and > the whole request fails, so I think it's worth checking. But if it's too slow > I can look at making it faster. Unfortunately, I cannot patch any upstream > project :) https://github.com/gluon-lang/languageserver-types/blob/master/src/lib.rs#L2016 LanguageClient-neovim returns empty candidate list if one of the candidates has unknown SymbolKind. Apparently they should be more tolerant and there is an issue tracking it. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle added inline comments. Comment at: clangd/FindSymbols.cpp:31 + + if (supportedSymbolKinds && + std::find(supportedSymbolKinds->begin(), supportedSymbolKinds->end(), MaskRay wrote: > This std::find loop adds some overhead to each candidate... In my experience > the client usually doesn't care about the returned symbol kinds, they are > used to give a category name. You can always patch the upstream to add > missing categories. > > This is one instance where LSP is over specified. nvm I don't have strong > opinion here I have a client that throws an exception when the symbolkind is not known and the whole request fails, so I think it's worth checking. But if it's too slow I can look at making it faster. Unfortunately, I cannot patch any upstream project :) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
sammccall added a comment.
This is fantastic, really looking forward to using it.
I'm going to need to give up on vim at this rate, or do some serious work on
ycm.
Most significant comments are around the new functions in SourceLocation, and
whether we can keep the type-mapping restricted to the LSP layer.
Comment at: clangd/ClangdLSPServer.cpp:99
Params.capabilities.textDocument.completion.completionItem.snippetSupport;
+ if (Params.capabilities.workspace && Params.capabilities.workspace->symbol &&
+ Params.capabilities.workspace->symbol->symbolKind)
malaperle wrote:
> sammccall wrote:
> > This is the analogue to the one on `completion` that we currently ignore,
> > and one on `symbol` corresponding to the `documentSymbol` call which isn't
> > implemented yet.
> >
> > I think the value of the extended types is low and it might be worth
> > leaving out of the first version to keep complexity down.
> > (If we really want it, the supporting code (mapping tables) should probably
> > be in a place where it can be reused by `documentSymbol` and can live next
> > to the equivalent for `completion`...)
> I think having Struct and EnumMember is nice and for sure once Macros is
> there we will want to use it. So would it be OK to move to FindSymbols.cpp
> (the new common file for document/symbol and workspace/symbol)?
> I think having Struct and EnumMember is nice and for sure once Macros is
> there we will want to use it.
OK, fair enough.
I think what bothers me about this option is how deep it goes - this isn't
really a request option in any useful sense.
I'd suggest the "C++ layer" i.e. ClangdServer should always return
full-fidelity results, and the "LSP layer" should deal with folding them.
In which case adjustKindToCapability should live somewhere like protocol.h and
get called from this file - WDYT?
Comment at: clangd/ClangdServer.h:161
+ void workspaceSymbols(StringRef Query,
+const clangd::WorkspaceSymbolOptions &Opts,
nit: add a comment for this API, e.g. `/// Retrieve the top symbols from the
workspace matching a query`
Comment at: clangd/FindSymbols.cpp:28
+ // All clients should support those.
+ if (Kind >= SymbolKind::File && Kind <= SymbolKind::Array)
+return Kind;
Can we rather have this condition checked when the client sets the supported
symbols, and remove this special knowledge here?
Comment at: clangd/FindSymbols.cpp:32
+ if (supportedSymbolKinds &&
+ std::find(supportedSymbolKinds->begin(), supportedSymbolKinds->end(),
+Kind) != supportedSymbolKinds->end())
this seems gratuituously inefficient - I guess it doesn't matter that much, but
can't we store the supported kinds as a std::bitset or so?
Comment at: clangd/FindSymbols.cpp:36
+
+ // Provide some fall backs for common kinds that are close enough.
+ if (Kind == SymbolKind::Struct)
nit: if we think this will be extended, a switch might be clearer
Comment at: clangd/FindSymbols.cpp:42
+
+ if (!supportedSymbolKinds) {
+// Provide some sensible default when all fails.
This code shouldn't need to handle this case. The LSP specifies the default set
of supported types if it's not provided, so ClangdLSPServer should enure we
always have a valid set
Comment at: clangd/FindSymbols.cpp:114
+ std::vector Result;
+ if (Query.empty() || !Index)
+return Result;
why Query.empty()?
In practice maybe showing results before you type is bad UX, but that seems up
to the editor to decide.
Comment at: clangd/FindSymbols.cpp:117
+
+ auto Names = splitQualifiedName(Query);
+
nit: move this below the next "paragraph" where it's used?
Comment at: clangd/FindSymbols.cpp:130
+
+ // Global scope is represented by "" in FuzzyFind.
+ if (Names.first.startswith("::"))
nit: this is `Names.first.consume_front("::")`
Might be clearer as "FuzzyFind doesn't want leading :: qualifier" to avoid
implication, that global namespace is special, particularly because...
Comment at: clangd/FindSymbols.cpp:130
+
+ // Global scope is represented by "" in FuzzyFind.
+ if (Names.first.startswith("::"))
sammccall wrote:
> nit: this is `Names.first.consume_front("::")`
> Might be clearer as "FuzzyFind doesn't want leading :: qualifier" to avoid
> implication, that global namespace is special, particularly because...
actually the global namespace *is* special :-)
IMO If you type `Foo`, you should get results from any namespace, but if you
type `::Foo` you should only get results from the global namespace.
So something like:
```
if (Names.first.consume_front("::") || !Names.first.empty())
Req.Scopes = {Names.first};
```
thou
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
MaskRay added inline comments.
Comment at: clangd/FindSymbols.cpp:31
+
+ if (supportedSymbolKinds &&
+ std::find(supportedSymbolKinds->begin(), supportedSymbolKinds->end(),
This std::find loop adds some overhead to each candidate... In my experience
the client usually doesn't care about the returned symbol kinds, they are used
to give a category name. You can always patch the upstream to add missing
categories.
This is one instance where LSP is over specified. nvm I don't have strong
opinion here
Comment at: clangd/Protocol.cpp:206
+
+bool fromJSON(const json::Expr &Params, WorkspaceClientCapabilities &R) {
+ json::ObjectMapper O(Params);
This copy-pasting exposes another problem that the current `fromJSON` `toJSON`
approach is too verbose and you may find other space-efficient serialization
format convenient Anyway, they can always be improved in the future.
Comment at: clangd/tool/ClangdMain.cpp:235
+ clangd::WorkspaceSymbolOptions WorkspaceSymOpts;
+ WorkspaceSymOpts.Limit = LimitResults;
I know command line options are easy for testing purpose but they are clumsy
for users. You need a "initialization option" <-> "command line option" bridge.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle added a comment. I realized that using the draft store has limited improvement for now because not many symbol locations come from main files (.cpp, etc) and when headers are modified, the main files are not reindexed right now. So the draft store will give better location for example when navigating to a function definition which moved in the unsaved main file. But it will be more general once header updates are handled better. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle updated this revision to Diff 140272.
malaperle added a comment.
Use the DraftStore for offset -> line/col when there is a draft available.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
Files:
clangd/CMakeLists.txt
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
clangd/index/SymbolCollector.cpp
clangd/tool/ClangdMain.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
unittests/clangd/CMakeLists.txt
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
@@ -41,6 +41,10 @@
std::string runDumpAST(ClangdServer &Server, PathRef File);
+llvm::Expected>
+runWorkspaceSymbols(ClangdServer &Server, StringRef Query,
+const WorkspaceSymbolOptions &Opts, const DraftStore &DS);
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -110,5 +110,13 @@
return std::move(*Result);
}
+llvm::Expected>
+runWorkspaceSymbols(ClangdServer &Server, StringRef Query,
+const WorkspaceSymbolOptions &Opts, const DraftStore &DS) {
+ llvm::Optional>> Result;
+ Server.workspaceSymbols(Query, Opts, DS, capture(Result));
+ return std::move(*Result);
+}
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- /dev/null
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -0,0 +1,383 @@
+//===-- FindSymbolsTests.cpp -*- C++ -*===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "Annotations.h"
+#include "ClangdServer.h"
+#include "DraftStore.h"
+#include "FindSymbols.h"
+#include "SyncAPI.h"
+#include "TestFS.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+void PrintTo(const SymbolInformation &I, std::ostream *O) {
+ llvm::raw_os_ostream OS(*O);
+ OS << I.containerName << I.name << " - " << toJSON(I);
+}
+
+namespace {
+
+using ::testing::AllOf;
+using ::testing::AnyOf;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAre;
+
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+ void onDiagnosticsReady(PathRef File,
+ std::vector Diagnostics) override {}
+};
+
+// GMock helpers for matching SymbolInfos items.
+MATCHER_P(Named, Name, "") { return arg.name == Name; }
+MATCHER_P(InContainer, ContainerName, "") {
+ return arg.containerName == ContainerName;
+}
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+
+class WorkspaceSymbolsTest : public ::testing::Test {
+protected:
+ std::unique_ptr FSProvider;
+ std::unique_ptr CDB;
+ std::unique_ptr DiagConsumer;
+ std::unique_ptr Server;
+ std::unique_ptr Opts;
+ std::unique_ptr DS;
+
+ virtual void SetUp() override {
+FSProvider = llvm::make_unique();
+CDB = llvm::make_unique();
+DiagConsumer = llvm::make_unique();
+auto ServerOpts = ClangdServer::optsForTest();
+ServerOpts.BuildDynamicSymbolIndex = true;
+Server = llvm::make_unique(*CDB, *FSProvider, *DiagConsumer,
+ ServerOpts);
+Opts = llvm::make_unique();
+DS = llvm::make_unique();
+ }
+
+ std::vector getSymbols(StringRef Query) {
+EXPECT_TRUE(Server->blockUntilIdleForTest()) << "Waiting for preamble";
+auto SymbolInfos = runWorkspaceSymbols(*Server, Query, *Opts, *DS);
+EXPECT_TRUE(bool(SymbolInfos)) << "workspaceSymbols returned an error";
+return *SymbolInfos;
+ }
+
+ void addFile(StringRef FileName, StringRef Contents) {
+auto Path = testPath(FileName);
+FSProvider->Files[Path] = Contents;
+Server->addDocument(Path, Contents);
+ }
+};
+
+} // namespace
+
+TEST_F(WorkspaceSymbolsTest, NoMacro) {
+ addFile("foo.cpp", R"cpp(
+ #define MACRO X
+ )cpp");
+
+ // Macros are not in the index.
+ EXPECT_THAT(getSymbols("macro"), IsEmpty());
+}
+
+TEST_F(WorkspaceSymbolsTest, NoLocals) {
+ addFile("foo.cpp", R"cpp(
+ void test() {
+struct LocalClass {};
+int local_var;
+ })cpp");
+ EXPECT_THAT(getSymbols("local_var"), IsEmpty());
+ EXPE
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle added inline comments. Comment at: clangd/SourceCode.cpp:110 + +llvm::Optional offsetRangeToLocation(SourceManager &SourceMgr, + StringRef File, MaskRay wrote: > May I ask a question about the conversion between SourceLocation and LSP > location? When the document is slightly out of sync with the indexed version, > what will be returned? I forgot to cover the case of the unsaved files, which are indexed in memory. It that what you mean? I'll update the patch to address by using the DraftStore when available. There is also the case where the file is not opened but outdated on disk. I don't think we can do much about it but make sure it doesn't crash :) At worst, the location might be off, and navigation will be momentarily affected, until the index can be updated with the file change. This is what I've noticed in other IDEs as well. Comment at: clangd/tool/ClangdMain.cpp:105 +static llvm::cl::opt LimitWorkspaceSymbolResult( +"workspace-symbol-limit", MaskRay wrote: > malaperle wrote: > > sammccall wrote: > > > the -completion-limit was mostly to control rollout, I'm not sure this > > > needs to be a flag. If it does, can we make it the same flag as > > > completions (and call it -limit or so?) > > I think it's quite similar to "completions", when you type just one letter > > for example, you can get a lot of results and a lot of JSON output. So it > > feels like the flag could apply to both completions and workspace symbols. > > How about -limit-resuts? I think just -limit might be a bit too general as > > we might want to limit other things. > Can these options be set by LSP initialization options? They could. Are you say they *should*? We could add it in DidChangeConfigurationParams/ClangdConfigurationParamsChange (workspace/didChangeConfiguration) if we need to. I haven't tried or needed to add it on the client side though. It's not 100% clear what should go in workspace/didChangeConfiguration but I think it should probably things that the user would change more often at run-time. I'm not sure how frequent this "limit" will be changed by the user. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle added a comment. The index changes are moved here: https://reviews.llvm.org/D44954 I haven't changed the patch yet though, I just removed it from this one. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
MaskRay added inline comments. Comment at: clangd/tool/ClangdMain.cpp:105 +static llvm::cl::opt LimitWorkspaceSymbolResult( +"workspace-symbol-limit", malaperle wrote: > sammccall wrote: > > the -completion-limit was mostly to control rollout, I'm not sure this > > needs to be a flag. If it does, can we make it the same flag as completions > > (and call it -limit or so?) > I think it's quite similar to "completions", when you type just one letter > for example, you can get a lot of results and a lot of JSON output. So it > feels like the flag could apply to both completions and workspace symbols. > How about -limit-resuts? I think just -limit might be a bit too general as we > might want to limit other things. Can these options be set by LSP initialization options? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
MaskRay added a comment. > Have you considered also allowing '.' and ' ' (space) as separators in the > request? Having additional separators doesn't really hurt complexity of the > implementation, but allows to switch between tools for different languages > easier. I would suggest allowing patterns containing space to match text without space, e.g. pattern `a b` can match text `aB`. The initial character of each word in the pattern should be seen as a `Head` position. This behavior matches many fuzzy matching plugins used in Emacs and Vim. Comment at: clangd/SourceCode.cpp:110 + +llvm::Optional offsetRangeToLocation(SourceManager &SourceMgr, + StringRef File, May I ask a question about the conversion between SourceLocation and LSP location? When the document is slightly out of sync with the indexed version, what will be returned? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle updated this revision to Diff 139968.
malaperle marked 4 inline comments as done.
malaperle added a comment.
Split the patch so that this part only has the workspace/symbol part
and the index model will be updated separately.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
Files:
clangd/CMakeLists.txt
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
clangd/index/SymbolCollector.cpp
clangd/tool/ClangdMain.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
unittests/clangd/CMakeLists.txt
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
@@ -41,6 +41,10 @@
std::string runDumpAST(ClangdServer &Server, PathRef File);
+llvm::Expected>
+runWorkspaceSymbols(ClangdServer &Server, StringRef Query,
+const WorkspaceSymbolOptions &Opts);
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -110,5 +110,13 @@
return std::move(*Result);
}
+llvm::Expected>
+runWorkspaceSymbols(ClangdServer &Server, StringRef Query,
+const WorkspaceSymbolOptions &Opts) {
+ llvm::Optional>> Result;
+ Server.workspaceSymbols(Query, Opts, capture(Result));
+ return std::move(*Result);
+}
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- /dev/null
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -0,0 +1,380 @@
+//===-- FindSymbolsTests.cpp -*- C++ -*===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "Annotations.h"
+#include "ClangdServer.h"
+#include "FindSymbols.h"
+#include "SyncAPI.h"
+#include "TestFS.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+void PrintTo(const SymbolInformation &I, std::ostream *O) {
+ llvm::raw_os_ostream OS(*O);
+ OS << I.containerName << I.name << " - " << toJSON(I);
+}
+
+namespace {
+
+using ::testing::AllOf;
+using ::testing::AnyOf;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAre;
+
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+ void onDiagnosticsReady(PathRef File,
+ std::vector Diagnostics) override {}
+};
+
+// GMock helpers for matching SymbolInfos items.
+MATCHER_P(Named, Name, "") { return arg.name == Name; }
+MATCHER_P(InContainer, ContainerName, "") {
+ return arg.containerName == ContainerName;
+}
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+
+class WorkspaceSymbolsTest : public ::testing::Test {
+protected:
+ std::unique_ptr FSProvider;
+ std::unique_ptr CDB;
+ std::unique_ptr DiagConsumer;
+ std::unique_ptr Server;
+ std::unique_ptr Opts;
+
+ virtual void SetUp() override {
+FSProvider = llvm::make_unique();
+CDB = llvm::make_unique();
+DiagConsumer = llvm::make_unique();
+auto ServerOpts = ClangdServer::optsForTest();
+ServerOpts.BuildDynamicSymbolIndex = true;
+Server = llvm::make_unique(*CDB, *FSProvider, *DiagConsumer,
+ ServerOpts);
+Opts = llvm::make_unique();
+ }
+
+ std::vector getSymbols(StringRef Query) {
+EXPECT_TRUE(Server->blockUntilIdleForTest()) << "Waiting for preamble";
+auto SymbolInfos = runWorkspaceSymbols(*Server, Query, *Opts);
+EXPECT_TRUE(bool(SymbolInfos)) << "workspaceSymbols returned an error";
+return *SymbolInfos;
+ }
+
+ void addFile(StringRef FileName, StringRef Contents) {
+auto Path = testPath(FileName);
+FSProvider->Files[Path] = Contents;
+Server->addDocument(Path, Contents);
+ }
+};
+
+} // namespace
+
+TEST_F(WorkspaceSymbolsTest, NoMacro) {
+ addFile("foo.cpp", R"cpp(
+ #define MACRO X
+ )cpp");
+
+ // Macros are not in the index.
+ EXPECT_THAT(getSymbols("macro"), IsEmpty());
+}
+
+TEST_F(WorkspaceSymbolsTest, NoLocals) {
+ addFile("foo.cpp", R"cpp(
+ void test() {
+struct LocalClass {};
+int local_var;
+ })cpp");
+ EXPECT_THAT(getSymbols("local_var"), IsEmpty());
+ EXPECT_THAT(getSymbols("LocalClass"), IsEmpty());
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle marked 9 inline comments as done. malaperle added a comment. In https://reviews.llvm.org/D44882#1048355, @sammccall wrote: > Can we split this patch up (it's pretty large anyway) and land the > `workspaceSymbols` implementation first, without changing the indexer > behavior? > > I think the indexer changes are in the right direction, but I think > "ForCompletion" isn't quite enough metadata, and don't want to block > everything on that. Sounds good. ForCompletion was more of a short term hack in my mind, we can try to split it and make proper metadata. >> We have a lot more features in mind that will make the index much bigger, >> like adding all occurrences (maybe not in the current YAML though). > > (tangential) > FWIW, I think *that* might be a case where we want quite a different API - I > think this is a file-major index operation (handful of results per file) > rather than a symbol-major one (results per symbol is essentially unbounded, > e.g. `std::string`). At the scale of google's internal codebase, this means > we need a different backend, and this may be the case for large open-source > codebases too (I know chromium hits scaling issues with indexers they try). > There are also other operations where results are files rather than symbols: > which files include file x, etc. Yeah, there needs to be an API to fetch things per file, such as the dependencies and the occurrences it contains (for updating when the file is deleted, etc). I'm not sure there needs to be an API that queries a file for a given USR though, I think that could be transparent to the caller and an implementation detail. The results could be grouped by file though. Good discussion for future patches :) >> But having options to control the amount of information indexed sounds good >> to me as there can be a lot of different project sizes and there can be >> different tradeoffs. I had in mind to an option for including/excluding the >> occurrences because without them, you lose workspace/references, call >> hierarchy, etc, but you still have code completion and workspace/symbol, >> document/symbol, etc while making the index much smaller. But more options >> sounds good. > > If we add options, someone has to actually set them. > Anything that's too big for google's internal index or similar can be > filtered out in the wrapping code (mapreduce) if the indexer exposes enough > information to do so. > We shouldn't have problems with the in-memory dynamic index. > Features that are off in the default static indexer configuration won't end > up getting used much. I think everything should be ON by default but we can run into issues that the user wants to reduce index size and help indexing speed. We had such options in CDT since there are quite a few things that can make it big (macro expansion steps, all references, etc). But this can be added as needed later. > So I can see a use for filters where the command-line indexer (YAML) would > run too slow otherwise, but a) let's cross that bridge when we come to it Sounds good. In https://reviews.llvm.org/D44882#1049007, @ilya-biryukov wrote: > >>> - Current `fuzzyFind` implementation can only match qualifiers strictly > >>> (i.e. st::vector won't match std::vector). Should we look into allowing > >>> fuzzy matches for this feature? (not in this patch, rather in the long > >>> term). > >> > >> I'm not sure, I'm thinking there should be a balance between how fuzzy it > >> it and how much noise it generates. Right now I find it a bit too fuzzy > >> since when I type "Draft" (to find DraftMgr), it picks up things like > >> DocumentRangeFormattingParams. Adding fuzziness to the namespace would > >> make this worse. Maybe with improved scoring it won't matter too much? > >> I'll try FuzzyMatcher and see. > > > > +1, I think experience with `workspaceSymbols` will help us answer this > > question. > > I was using an IDE that had fuzzy find for an equivalent of > `workspaceSymbols` and found it to be an amazing experience. And having > consistent behavior between different features is really nice. > Good ranking is a key to it being useful, though. If when typing `Draft` you > get `DocumentRangeFormattingParams` ranked higher than `DraftMgr` that's a > bug in FuzzyMatcher. If you have some not-very-nice results at the end of the > list, this shouldn't be a problem in most cases. > > I'm highly in favor of enabling fuzzy matching for `workspaceSymbols`. I think it will be fine once we address the bug(s). As long as the exact matches are always first then there's no problem. Comment at: clangd/ClangdLSPServer.cpp:99 Params.capabilities.textDocument.completion.completionItem.snippetSupport; + if (Params.capabilities.workspace && Params.capabilities.workspace->symbol && + Params.capabilities.workspace->symbol->symbolKind) sammccall wrote: > This is the analogue to the on
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
ilya-biryukov added a comment. > I'm highly in favor of enabling fuzzy matching for `workspaceSymbols`. At lest for the name themselves. Non-fuzzy matching of qualifiers does not look that important. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
ilya-biryukov added a comment. >>> - Current `fuzzyFind` implementation can only match qualifiers strictly >>> (i.e. st::vector won't match std::vector). Should we look into allowing >>> fuzzy matches for this feature? (not in this patch, rather in the long >>> term). >> >> I'm not sure, I'm thinking there should be a balance between how fuzzy it it >> and how much noise it generates. Right now I find it a bit too fuzzy since >> when I type "Draft" (to find DraftMgr), it picks up things like >> DocumentRangeFormattingParams. Adding fuzziness to the namespace would make >> this worse. Maybe with improved scoring it won't matter too much? I'll try >> FuzzyMatcher and see. > > +1, I think experience with `workspaceSymbols` will help us answer this > question. I was using an IDE that had fuzzy find for an equivalent of `workspaceSymbols` and found it to be an amazing experience. And having consistent behavior between different features is really nice. Good ranking is a key to it being useful, though. If when typing `Draft` you get `DocumentRangeFormattingParams` ranked higher than `DraftMgr` that's a bug in FuzzyMatcher. If you have some not-very-nice results at the end of the list, this shouldn't be a problem in most cases. I'm highly in favor of enabling fuzzy matching for `workspaceSymbols`. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
sammccall added a comment. Very nice! I'd like to reduce the scope of the initial patch, which seems to be possible, so we can review the details but not get bogged down too much. In https://reviews.llvm.org/D44882#1048179, @malaperle wrote: > In https://reviews.llvm.org/D44882#1048043, @ilya-biryukov wrote: > > > - Unconditionally storing much more symbols in the index might have subtle > > performance implications, especially for our internal use (the codebase is > > huge). I bet that internally we wouldn't want to store the symbols not > > needed for completion, so we'll probably need a switch to disable storing > > them in the indexing implementation. But let's wait for Sam to take a look, > > he certainly has a better perspective on the issues there. > > > I'm a bit surprised that internally you do not want symbols beyond the ones > for completion. FWIW, I think we do, it just needs to be done carefully (with the right semantic filters at query time, and metadata at indexing time). Can we split this patch up (it's pretty large anyway) and land the `workspaceSymbols` implementation first, without changing the indexer behavior? I think the indexer changes are in the right direction, but I think "ForCompletion" isn't quite enough metadata, and don't want to block everything on that. > We have a lot more features in mind that will make the index much bigger, > like adding all occurrences (maybe not in the current YAML though). (tangential) FWIW, I think *that* might be a case where we want quite a different API - I think this is a file-major index operation (handful of results per file) rather than a symbol-major one (results per symbol is essentially unbounded, e.g. `std::string`). At the scale of google's internal codebase, this means we need a different backend, and this may be the case for large open-source codebases too (I know chromium hits scaling issues with indexers they try). There are also other operations where results are files rather than symbols: which files include file x, etc. > But having options to control the amount of information indexed sounds good > to me as there can be a lot of different project sizes and there can be > different tradeoffs. I had in mind to an option for including/excluding the > occurrences because without them, you lose workspace/references, call > hierarchy, etc, but you still have code completion and workspace/symbol, > document/symbol, etc while making the index much smaller. But more options > sounds good. If we add options, someone has to actually set them. Anything that's too big for google's internal index or similar can be filtered out in the wrapping code (mapreduce) if the indexer exposes enough information to do so. We shouldn't have problems with the in-memory dynamic index. Features that are off in the default static indexer configuration won't end up getting used much. So I can see a use for filters where the command-line indexer (YAML) would run too slow otherwise, but a) let's cross that bridge when we come to it and b) there's lots of low-hanging fruit there - the YAML format itself should be considered a placeholder. >> - Current `fuzzyFind` implementation can only match qualifiers strictly >> (i.e. st::vector won't match std::vector). Should we look into allowing >> fuzzy matches for this feature? (not in this patch, rather in the long >> term). > > I'm not sure, I'm thinking there should be a balance between how fuzzy it it > and how much noise it generates. Right now I find it a bit too fuzzy since > when I type "Draft" (to find DraftMgr), it picks up things like > DocumentRangeFormattingParams. Adding fuzziness to the namespace would make > this worse. Maybe with improved scoring it won't matter too much? I'll try > FuzzyMatcher and see. +1, I think experience with `workspaceSymbols` will help us answer this question. Comment at: clangd/ClangdLSPServer.cpp:99 Params.capabilities.textDocument.completion.completionItem.snippetSupport; + if (Params.capabilities.workspace && Params.capabilities.workspace->symbol && + Params.capabilities.workspace->symbol->symbolKind) This is the analogue to the one on `completion` that we currently ignore, and one on `symbol` corresponding to the `documentSymbol` call which isn't implemented yet. I think the value of the extended types is low and it might be worth leaving out of the first version to keep complexity down. (If we really want it, the supporting code (mapping tables) should probably be in a place where it can be reused by `documentSymbol` and can live next to the equivalent for `completion`...) Comment at: clangd/ClangdLSPServer.h:95 bool IsDone = false; + /// Indicates whether or not the index is available. + bool SymbolIndexAvailable = false; This comment is just the var name, delete? Comment at: clangd/ClangdLSPServer.h:96 +
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle added inline comments.
Comment at: clangd/WorkspaceSymbols.cpp:165
+[](const SymbolInformation &A, const SymbolInformation &B) {
+ return A.name < B.name;
+});
ilya-biryukov wrote:
> We have `FuzzyMatcher`, it can produce decent match scores and is already
> used in completion.
> Any reason not to use it here?
I think it was giving odd ordering but let me try this again and at least
document the reason.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle added a comment. In https://reviews.llvm.org/D44882#1048043, @ilya-biryukov wrote: > - Unconditionally storing much more symbols in the index might have subtle > performance implications, especially for our internal use (the codebase is > huge). I bet that internally we wouldn't want to store the symbols not > needed for completion, so we'll probably need a switch to disable storing > them in the indexing implementation. But let's wait for Sam to take a look, > he certainly has a better perspective on the issues there. I'm a bit surprised that internally you do not want symbols beyond the ones for completion. We have a lot more features in mind that will make the index much bigger, like adding all occurrences (maybe not in the current YAML though). But having options to control the amount of information indexed sounds good to me as there can be a lot of different project sizes and there can be different tradeoffs. I had in mind to an option for including/excluding the occurrences because without them, you lose workspace/references, call hierarchy, etc, but you still have code completion and workspace/symbol, document/symbol, etc while making the index much smaller. But more options sounds good. > - Current `fuzzyFind` implementation can only match qualifiers strictly (i.e. > st::vector won't match std::vector). Should we look into allowing fuzzy > matches for this feature? (not in this patch, rather in the long term). I'm not sure, I'm thinking there should be a balance between how fuzzy it it and how much noise it generates. Right now I find it a bit too fuzzy since when I type "Draft" (to find DraftMgr), it picks up things like DocumentRangeFormattingParams. Adding fuzziness to the namespace would make this worse. Maybe with improved scoring it won't matter too much? I'll try FuzzyMatcher and see. > - Have you considered also allowing `'.'` and `' '` (space) as separators in > the request? Having additional separators doesn't really hurt complexity of > the implementation, but allows to switch between tools for different > languages easier. E.g., `std.vector.push_back` and `std vector push_back` > could produce the same matches as `std::vector::push_back`. I haven't thought of that and I think it's a good idea. Sounds like a good, isolated thing to do in a separate patch. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
ilya-biryukov added reviewers: sammccall, ilya-biryukov.
ilya-biryukov added a subscriber: sammccall.
ilya-biryukov added a comment.
Adding @sammccall, he will definitely want to take a look at index-related
changes.
On a high level, the approach seems just right.
A few questions immedieately that came to mind:
- Unconditionally storing much more symbols in the index might have subtle
performance implications, especially for our internal use (the codebase is
huge). I bet that internally we wouldn't want to store the symbols not needed
for completion, so we'll probably need a switch to disable storing them in the
indexing implementation. But let's wait for Sam to take a look, he certainly
has a better perspective on the issues there.
- Current `fuzzyFind` implementation can only match qualifiers strictly (i.e.
st::vector won't match std::vector). Should we look into allowing fuzzy matches
for this feature? (not in this patch, rather in the long term).
- Have you considered also allowing `'.'` and `' '` (space) as separators in
the request? Having additional separators doesn't really hurt complexity of the
implementation, but allows to switch between tools for different languages
easier.
E.g., `std.vector.push_back` and `std vector push_back` could produce the same
matches as `std::vector::push_back`.
Comment at: clangd/ClangdServer.h:163
+ StringRef Query, const clangd::WorkspaceSymbolOptions &Opts,
+ UniqueFunction>)>
+ Callback);
NIT: use `Callback` typedef.
Comment at: clangd/WorkspaceSymbols.cpp:165
+[](const SymbolInformation &A, const SymbolInformation &B) {
+ return A.name < B.name;
+});
We have `FuzzyMatcher`, it can produce decent match scores and is already used
in completion.
Any reason not to use it here?
Comment at: clangd/index/Index.h:141
unsigned References = 0;
+ // Whether or not the symbol should be considered for completion.
NIT: remove empty lines to be consistent with the rest of the struct.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44882: [clangd] Implementation of workspace/symbol request
malaperle created this revision.
Herald added subscribers: cfe-commits, MaskRay, ioeric, jkorous-apple, mgrang,
ilya-biryukov, mgorny, klimek.
This is a basic implementation of the "workspace/symbol" request which is
used to find symbols by a string query. Since this is similar to code completion
in terms of result, this implementation reuses the "fuzzyFind" in order to get
matches. The index model was augmented to include more symbols like class
members so that those can also be found by fuzzyFind and therefore
workspace/symbol. Because fuzzyFind can now returns more results, a new option
flag is introduced to optionally narrow down results to only completion-matches;
this is how code completion can still behave the way it did before.
For now, results are sorted alphanumerically and improvements to scoring could
be done in the future.
Another further improvement would be to include symbols in anonymous namespaces.
Signed-off-by: Marc-Andre Laperle
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44882
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/CodeComplete.cpp
clangd/Protocol.cpp
clangd/Protocol.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/WorkspaceSymbols.cpp
clangd/WorkspaceSymbols.h
clangd/XRefs.cpp
clangd/index/Index.h
clangd/index/MemIndex.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolYAML.cpp
clangd/tool/ClangdMain.cpp
test/clangd/initialize-params-invalid.test
test/clangd/initialize-params.test
unittests/clangd/CMakeLists.txt
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
unittests/clangd/SyncAPI.cpp
unittests/clangd/SyncAPI.h
unittests/clangd/WorkspaceSymbolsTests.cpp
Index: unittests/clangd/WorkspaceSymbolsTests.cpp
===
--- /dev/null
+++ unittests/clangd/WorkspaceSymbolsTests.cpp
@@ -0,0 +1,373 @@
+//===-- WorkspaceSymbolsTests.cpp -*- C++ -*---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include "Annotations.h"
+#include "ClangdServer.h"
+#include "SyncAPI.h"
+#include "TestFS.h"
+#include "WorkspaceSymbols.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+
+void PrintTo(const SymbolInformation &I, std::ostream *O) {
+ llvm::raw_os_ostream OS(*O);
+ OS << I.containerName << I.name << " - " << toJSON(I);
+}
+
+namespace {
+
+using ::testing::AllOf;
+using ::testing::AnyOf;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAre;
+
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+ void onDiagnosticsReady(PathRef File,
+ std::vector Diagnostics) override {}
+};
+
+// GMock helpers for matching SymbolInfos items.
+MATCHER_P(Named, Name, "") { return arg.name == Name; }
+MATCHER_P(InContainer, ContainerName, "") {
+ return arg.containerName == ContainerName;
+}
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+
+class WorkspaceSymbolsTest : public ::testing::Test {
+protected:
+ std::unique_ptr FSProvider;
+ std::unique_ptr CDB;
+ std::unique_ptr DiagConsumer;
+ std::unique_ptr Server;
+ std::unique_ptr Opts;
+
+ virtual void SetUp() override {
+FSProvider = llvm::make_unique();
+CDB = llvm::make_unique();
+DiagConsumer = llvm::make_unique();
+auto ServerOpts = ClangdServer::optsForTest();
+ServerOpts.BuildDynamicSymbolIndex = true;
+Server = llvm::make_unique(*CDB, *FSProvider, *DiagConsumer,
+ ServerOpts);
+Opts = llvm::make_unique();
+ }
+
+ std::vector getSymbols(StringRef Query) {
+EXPECT_TRUE(Server->blockUntilIdleForTest()) << "Waiting for preamble";
+auto SymbolInfos = runWorkspaceSymbols(*Server, Query, *Opts);
+EXPECT_TRUE(bool(SymbolInfos)) << "workspaceSymbols returned an error";
+return *SymbolInfos;
+ }
+
+ void addFile(StringRef FileName, StringRef Contents) {
+auto Path = testPath(FileName);
+FSProvider->Files[Path] = Contents;
+Server->addDocument(Path, Contents);
+ }
+};
+
+} // namespace
+
+TEST_F(WorkspaceSymbolsTest, NoMacro) {
+ addFile("foo.cpp", R"cpp(
+ #define MACRO X
+ )cpp");
+
+ // Macros are not in the index.
+ EXPECT_THAT(getSymbols("macro"), IsEmpty());
+}
+
+TEST_F(WorkspaceSymbolsTest, NoLocals) {
+ addFile("foo.cpp", R"cpp(
+ void test() {
+struct LocalClass {};
+int local_var;
+ })cpp");
+ EXPECT_THAT(getSymbols("local_var"), IsEmpty());
+ EXPECT_THAT(getSy
