[PATCH] D36150: [clangd] LSP extension to switch between source/header file
ilya-biryukov added a comment. Sorry for delays @malaperle, thanks for submitting the patch. Repository: rL LLVM https://reviews.llvm.org/D36150 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
malaperle added a comment. I submitted the patch. Thanks a lot William and Ilya! Repository: rL LLVM https://reviews.llvm.org/D36150 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
This revision was automatically updated to reflect the committed changes.
Closed by commit rL314377: [clangd] LSP extension to switch between
source/header file (authored by malaperle).
Changed prior to commit:
https://reviews.llvm.org/D36150?vs=116387=116919#toc
Repository:
rL LLVM
https://reviews.llvm.org/D36150
Files:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
clang-tools-extra/trunk/clangd/ProtocolHandlers.h
clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
@@ -7,6 +7,7 @@
//
//===--===//
+#include "ClangdLSPServer.h"
#include "ClangdServer.h"
#include "Logger.h"
#include "clang/Basic/VirtualFileSystem.h"
@@ -899,6 +900,84 @@
}
}
+TEST_F(ClangdVFSTest, CheckSourceHeaderSwitch) {
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
+
+ ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+ /*SnippetCompletions=*/false, EmptyLogger::getInstance());
+
+ auto SourceContents = R"cpp(
+ #include "foo.h"
+ int b = a;
+ )cpp";
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ auto FooH = getVirtualTestFilePath("foo.h");
+ auto Invalid = getVirtualTestFilePath("main.cpp");
+
+ FS.Files[FooCpp] = SourceContents;
+ FS.Files[FooH] = "int a;";
+ FS.Files[Invalid] = "int main() { \n return 0; \n }";
+
+ llvm::Optional PathResult = Server.switchSourceHeader(FooCpp);
+ EXPECT_TRUE(PathResult.hasValue());
+ ASSERT_EQ(PathResult.getValue(), FooH);
+
+ PathResult = Server.switchSourceHeader(FooH);
+ EXPECT_TRUE(PathResult.hasValue());
+ ASSERT_EQ(PathResult.getValue(), FooCpp);
+
+ SourceContents = R"c(
+ #include "foo.HH"
+ int b = a;
+ )c";
+
+ // Test with header file in capital letters and different extension, source
+ // file with different extension
+ auto FooC = getVirtualTestFilePath("bar.c");
+ auto FooHH = getVirtualTestFilePath("bar.HH");
+
+ FS.Files[FooC] = SourceContents;
+ FS.Files[FooHH] = "int a;";
+
+ PathResult = Server.switchSourceHeader(FooC);
+ EXPECT_TRUE(PathResult.hasValue());
+ ASSERT_EQ(PathResult.getValue(), FooHH);
+
+ // Test with both capital letters
+ auto Foo2C = getVirtualTestFilePath("foo2.C");
+ auto Foo2HH = getVirtualTestFilePath("foo2.HH");
+ FS.Files[Foo2C] = SourceContents;
+ FS.Files[Foo2HH] = "int a;";
+
+ PathResult = Server.switchSourceHeader(Foo2C);
+ EXPECT_TRUE(PathResult.hasValue());
+ ASSERT_EQ(PathResult.getValue(), Foo2HH);
+
+ // Test with source file as capital letter and .hxx header file
+ auto Foo3C = getVirtualTestFilePath("foo3.C");
+ auto Foo3HXX = getVirtualTestFilePath("foo3.hxx");
+
+ SourceContents = R"c(
+ #include "foo3.hxx"
+ int b = a;
+ )c";
+
+ FS.Files[Foo3C] = SourceContents;
+ FS.Files[Foo3HXX] = "int a;";
+
+ PathResult = Server.switchSourceHeader(Foo3C);
+ EXPECT_TRUE(PathResult.hasValue());
+ ASSERT_EQ(PathResult.getValue(), Foo3HXX);
+
+ // Test if asking for a corresponding file that doesn't exist returns an empty
+ // string.
+ PathResult = Server.switchSourceHeader(Invalid);
+ EXPECT_FALSE(PathResult.hasValue());
+}
+
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
public:
Index: clang-tools-extra/trunk/clangd/ProtocolHandlers.h
===
--- clang-tools-extra/trunk/clangd/ProtocolHandlers.h
+++ clang-tools-extra/trunk/clangd/ProtocolHandlers.h
@@ -49,6 +49,8 @@
JSONOutput ) = 0;
virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
JSONOutput ) = 0;
+ virtual void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
+JSONOutput ) = 0;
};
void regiterCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clang-tools-extra/trunk/clangd/ClangdServer.h
===
--- clang-tools-extra/trunk/clangd/ClangdServer.h
+++ clang-tools-extra/trunk/clangd/ClangdServer.h
@@ -248,6 +248,10 @@
/// Get definition of symbol at a specified \p Line and \p Column in \p File.
Tagged findDefinitions(PathRef File, Position Pos);
+ /// Helper function that returns a path to the corresponding source file when
+ /// given a header file and vice versa.
+ llvm::Optional switchSourceHeader(PathRef
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
Nebiroth added a comment. In https://reviews.llvm.org/D36150#879194, @ilya-biryukov wrote: > Looks good. > Do you want me to submit this patch for you? Yes please. https://reviews.llvm.org/D36150 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
ilya-biryukov accepted this revision. ilya-biryukov added a comment. Looks good. Do you want me to submit this patch for you? https://reviews.llvm.org/D36150 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
Nebiroth updated this revision to Diff 116387.
Nebiroth marked 2 inline comments as done.
Nebiroth added a comment.
Rebased on latest version.
Corrected code style issues in test file.
https://reviews.llvm.org/D36150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -7,6 +7,7 @@
//
//===--===//
+#include "ClangdLSPServer.h"
#include "ClangdServer.h"
#include "Logger.h"
#include "clang/Basic/VirtualFileSystem.h"
@@ -899,6 +900,84 @@
}
}
+TEST_F(ClangdVFSTest, CheckSourceHeaderSwitch) {
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
+
+ ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+ /*SnippetCompletions=*/false, EmptyLogger::getInstance());
+
+ auto SourceContents = R"cpp(
+ #include "foo.h"
+ int b = a;
+ )cpp";
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ auto FooH = getVirtualTestFilePath("foo.h");
+ auto Invalid = getVirtualTestFilePath("main.cpp");
+
+ FS.Files[FooCpp] = SourceContents;
+ FS.Files[FooH] = "int a;";
+ FS.Files[Invalid] = "int main() { \n return 0; \n }";
+
+ llvm::Optional PathResult = Server.switchSourceHeader(FooCpp);
+ EXPECT_TRUE(PathResult.hasValue());
+ ASSERT_EQ(PathResult.getValue(), FooH);
+
+ PathResult = Server.switchSourceHeader(FooH);
+ EXPECT_TRUE(PathResult.hasValue());
+ ASSERT_EQ(PathResult.getValue(), FooCpp);
+
+ SourceContents = R"c(
+ #include "foo.HH"
+ int b = a;
+ )c";
+
+ // Test with header file in capital letters and different extension, source
+ // file with different extension
+ auto FooC = getVirtualTestFilePath("bar.c");
+ auto FooHH = getVirtualTestFilePath("bar.HH");
+
+ FS.Files[FooC] = SourceContents;
+ FS.Files[FooHH] = "int a;";
+
+ PathResult = Server.switchSourceHeader(FooC);
+ EXPECT_TRUE(PathResult.hasValue());
+ ASSERT_EQ(PathResult.getValue(), FooHH);
+
+ // Test with both capital letters
+ auto Foo2C = getVirtualTestFilePath("foo2.C");
+ auto Foo2HH = getVirtualTestFilePath("foo2.HH");
+ FS.Files[Foo2C] = SourceContents;
+ FS.Files[Foo2HH] = "int a;";
+
+ PathResult = Server.switchSourceHeader(Foo2C);
+ EXPECT_TRUE(PathResult.hasValue());
+ ASSERT_EQ(PathResult.getValue(), Foo2HH);
+
+ // Test with source file as capital letter and .hxx header file
+ auto Foo3C = getVirtualTestFilePath("foo3.C");
+ auto Foo3HXX = getVirtualTestFilePath("foo3.hxx");
+
+ SourceContents = R"c(
+ #include "foo3.hxx"
+ int b = a;
+ )c";
+
+ FS.Files[Foo3C] = SourceContents;
+ FS.Files[Foo3HXX] = "int a;";
+
+ PathResult = Server.switchSourceHeader(Foo3C);
+ EXPECT_TRUE(PathResult.hasValue());
+ ASSERT_EQ(PathResult.getValue(), Foo3HXX);
+
+ // Test if asking for a corresponding file that doesn't exist returns an empty
+ // string.
+ PathResult = Server.switchSourceHeader(Invalid);
+ EXPECT_FALSE(PathResult.hasValue());
+}
+
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
public:
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -48,6 +48,8 @@
JSONOutput ) = 0;
virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
JSONOutput ) = 0;
+ virtual void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
+JSONOutput ) = 0;
};
void regiterCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -204,6 +204,22 @@
ProtocolCallbacks
};
+struct SwitchSourceHeaderHandler : Handler {
+ SwitchSourceHeaderHandler(JSONOutput , ProtocolCallbacks )
+ : Handler(Output), Callbacks(Callbacks) {}
+
+ void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
+auto TDPP = TextDocumentIdentifier::parse(Params, Output);
+if (!TDPP)
+ return;
+
+Callbacks.onSwitchSourceHeader(*TDPP, ID, Output);
+ }
+
+private:
+ ProtocolCallbacks
+};
+
} // namespace
void clangd::regiterCallbackHandlers(JSONRPCDispatcher ,
@@ -240,4 +256,7 @@
Dispatcher.registerHandler(
"textDocument/definition",
llvm::make_unique(Out, Callbacks));
+ Dispatcher.registerHandler(
+ "textDocument/switchSourceHeader",
+
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
Looks good modulo a few NITS.
Let me know if you need help landing this.
Comment at: unittests/clangd/ClangdTests.cpp:910
+ auto FooH = getVirtualTestFilePath("foo.h");
+ auto invalid = getVirtualTestFilePath("main.cpp");
+
Code style: local variables are `UpperCamelCase`
Comment at: unittests/clangd/ClangdTests.cpp:916
+
+ llvm::Optional pathResult = Server.switchSourceHeader(FooCpp);
+ EXPECT_TRUE(pathResult.hasValue());
Code style: local variables are `UpperCamelCase`
Comment at: unittests/clangd/ClangdTests.cpp:967
+
+ // Test if asking for a corresponding file that doesn't exist returns an
empty string.
+ pathResult = Server.switchSourceHeader(invalid);
The text is over the line limit.
Could you please `clang-format` every submission?
https://reviews.llvm.org/D36150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
Nebiroth updated this revision to Diff 116038.
Nebiroth added a comment.
Added unit test.
https://reviews.llvm.org/D36150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -892,5 +892,83 @@
}
}
+TEST_F(ClangdVFSTest, CheckSourceHeaderSwitch) {
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
+
+ ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+ /*SnippetCompletions=*/false);
+
+ auto SourceContents = R"cpp(
+ #include "foo.h"
+ int b = a;
+ )cpp";
+
+ auto FooCpp = getVirtualTestFilePath("foo.cpp");
+ auto FooH = getVirtualTestFilePath("foo.h");
+ auto invalid = getVirtualTestFilePath("main.cpp");
+
+ FS.Files[FooCpp] = SourceContents;
+ FS.Files[FooH] = "int a;";
+ FS.Files[invalid] = "int main() { \n return 0; \n }";
+
+ llvm::Optional pathResult = Server.switchSourceHeader(FooCpp);
+ EXPECT_TRUE(pathResult.hasValue());
+ ASSERT_EQ(pathResult.getValue(), FooH);
+
+ pathResult = Server.switchSourceHeader(FooH);
+ EXPECT_TRUE(pathResult.hasValue());
+ ASSERT_EQ(pathResult.getValue(), FooCpp);
+
+ SourceContents = R"c(
+ #include "foo.HH"
+ int b = a;
+ )c";
+
+ // Test with header file in capital letters and different extension, source file with different extension
+ auto FooC = getVirtualTestFilePath("bar.c");
+ auto FooHH = getVirtualTestFilePath("bar.HH");
+
+ FS.Files[FooC] = SourceContents;
+ FS.Files[FooHH] = "int a;";
+
+ pathResult = Server.switchSourceHeader(FooC);
+ EXPECT_TRUE(pathResult.hasValue());
+ ASSERT_EQ(pathResult.getValue(), FooHH);
+
+ // Test with both capital letters
+ auto Foo2C = getVirtualTestFilePath("foo2.C");
+ auto Foo2HH = getVirtualTestFilePath("foo2.HH");
+ FS.Files[Foo2C] = SourceContents;
+ FS.Files[Foo2HH] = "int a;";
+
+ pathResult = Server.switchSourceHeader(Foo2C);
+ EXPECT_TRUE(pathResult.hasValue());
+ ASSERT_EQ(pathResult.getValue(), Foo2HH);
+
+ // Test with source file as capital letter and .hxx header file
+ auto Foo3C = getVirtualTestFilePath("foo3.C");
+ auto Foo3HXX = getVirtualTestFilePath("foo3.hxx");
+
+ SourceContents = R"c(
+ #include "foo3.hxx"
+ int b = a;
+ )c";
+
+ FS.Files[Foo3C] = SourceContents;
+ FS.Files[Foo3HXX] = "int a;";
+
+ pathResult = Server.switchSourceHeader(Foo3C);
+ EXPECT_TRUE(pathResult.hasValue());
+ ASSERT_EQ(pathResult.getValue(), Foo3HXX);
+
+
+ // Test if asking for a corresponding file that doesn't exist returns an empty string.
+ pathResult = Server.switchSourceHeader(invalid);
+ EXPECT_FALSE(pathResult.hasValue());
+
+}
+
} // namespace clangd
} // namespace clang
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -48,6 +48,8 @@
JSONOutput ) = 0;
virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
JSONOutput ) = 0;
+ virtual void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
+JSONOutput ) = 0;
};
void regiterCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -204,6 +204,22 @@
ProtocolCallbacks
};
+struct SwitchSourceHeaderHandler : Handler {
+ SwitchSourceHeaderHandler(JSONOutput , ProtocolCallbacks )
+ : Handler(Output), Callbacks(Callbacks) {}
+
+ void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
+auto TDPP = TextDocumentIdentifier::parse(Params);
+if (!TDPP)
+ return;
+
+Callbacks.onSwitchSourceHeader(*TDPP, ID, Output);
+ }
+
+private:
+ ProtocolCallbacks
+};
+
} // namespace
void clangd::regiterCallbackHandlers(JSONRPCDispatcher ,
@@ -240,4 +256,7 @@
Dispatcher.registerHandler(
"textDocument/definition",
llvm::make_unique(Out, Callbacks));
+ Dispatcher.registerHandler(
+ "textDocument/switchSourceHeader",
+ llvm::make_unique(Out, Callbacks));
}
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -241,6 +241,10 @@
/// Get definition of symbol at a specified \p Line and \p Column in \p File.
Tagged findDefinitions(PathRef File, Position Pos);
+ /// Helper function that returns a path to the corresponding source file when
+ ///
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
ilya-biryukov added a comment. In https://reviews.llvm.org/D36150#875623, @Nebiroth wrote: > In https://reviews.llvm.org/D36150#867971, @ilya-biryukov wrote: > > > Overall looks good, but still needs at least a few tests. > > > I have a test for this commit that uses included source and header files, > would that be okay to do or should I generate files dynamically? If so, how > can I accomplish this? You are right, unfortunately we don't have a good story for multi-file tests yet. Let's add a unit-test for the implementation in `ClangdServer`. This can be easily done by using `MockFSProvider`. You may find some examples in `clang-tools-extra/unittest/clangd/ClangdTests.cpp` https://reviews.llvm.org/D36150 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
Nebiroth added a comment. In https://reviews.llvm.org/D36150#867971, @ilya-biryukov wrote: > Overall looks good, but still needs at least a few tests. I have a test for this commit that uses included source and header files, would that be okay to do or should I generate files dynamically? If so, how can I accomplish this? https://reviews.llvm.org/D36150 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. Overall looks good, but still needs at least a few tests. https://reviews.llvm.org/D36150 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
Nebiroth updated this revision to Diff 114652.
Nebiroth added a comment.
Return value when no file is found is now an empty string instead of file://
Minor code style changes and cleanup.
Ran clang-format on ClangdServer.h
https://reviews.llvm.org/D36150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
clangd/tool/ClangdMain.cpp
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -12,7 +12,6 @@
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Program.h"
-
#include
#include
#include
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -47,7 +47,9 @@
virtual void onCompletion(TextDocumentPositionParams Params, StringRef ID,
JSONOutput ) = 0;
virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
-JSONOutput ) = 0;
+JSONOutput ) = 0;
+ virtual void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
+JSONOutput ) = 0;
};
void regiterCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -204,6 +204,22 @@
ProtocolCallbacks
};
+struct SwitchSourceHeaderHandler : Handler {
+ SwitchSourceHeaderHandler(JSONOutput , ProtocolCallbacks )
+ : Handler(Output), Callbacks(Callbacks) {}
+
+ void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
+auto TDPP = TextDocumentIdentifier::parse(Params);
+if (!TDPP)
+ return;
+
+Callbacks.onSwitchSourceHeader(*TDPP, ID, Output);
+ }
+
+private:
+ ProtocolCallbacks
+};
+
} // namespace
void clangd::regiterCallbackHandlers(JSONRPCDispatcher ,
@@ -237,6 +253,10 @@
Dispatcher.registerHandler(
"textDocument/completion",
llvm::make_unique(Out, Callbacks));
- Dispatcher.registerHandler("textDocument/definition",
+ Dispatcher.registerHandler(
+ "textDocument/definition",
llvm::make_unique(Out, Callbacks));
+ Dispatcher.registerHandler(
+ "textDocument/switchSourceHeader",
+ llvm::make_unique(Out, Callbacks));
}
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -182,6 +182,10 @@
/// Get definition of symbol at a specified \p Line and \p Column in \p File.
Tagged findDefinitions(PathRef File, Position Pos);
+ /// Helper function that returns a path to the corresponding source file when
+ /// given a header file and vice versa.
+ llvm::Optional switchSourceHeader(PathRef Path);
+
/// Run formatting for \p Rng inside \p File.
std::vector formatRange(PathRef File, Range Rng);
/// Run formatting for the whole \p File.
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -15,6 +15,7 @@
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/raw_ostream.h"
#include
@@ -273,16 +274,76 @@
return DumpFuture.get();
}
-Tagged
-ClangdServer::findDefinitions(PathRef File, Position Pos) {
+Tagged ClangdServer::findDefinitions(PathRef File,
+Position Pos) {
auto FileContents = DraftMgr.getDraft(File);
- assert(FileContents.Draft && "findDefinitions is called for non-added document");
+ assert(FileContents.Draft &&
+ "findDefinitions is called for non-added document");
std::vector Result;
auto TaggedFS = FSProvider.getTaggedFileSystem(File);
- Units.runOnUnit(File, *FileContents.Draft, ResourceDir, CDB, PCHs,
- TaggedFS.Value, [&](ClangdUnit ) {
-Result = Unit.findDefinitions(Pos);
- });
+ Units.runOnUnit(
+ File, *FileContents.Draft, ResourceDir, CDB, PCHs, TaggedFS.Value,
+ [&](ClangdUnit ) { Result = Unit.findDefinitions(Pos); });
return make_tagged(std::move(Result), TaggedFS.Tag);
}
+
+llvm::Optional ClangdServer::switchSourceHeader(PathRef Path) {
+
+ StringRef SourceExtensions[] = {".cpp", ".c", ".cc", ".cxx",
+ ".c++", ".m", ".mm"};
+ StringRef HeaderExtensions[] = {".h", ".hh", ".hpp", ".hxx", ".inc"};
+
+ StringRef PathExt = llvm::sys::path::extension(Path);
+
+ // Lookup in a list of known extensions.
+ auto SourceIter =
+
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
Nebiroth marked 5 inline comments as done.
Nebiroth added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:234
+ else
+ResultUri = URI::fromFile("");
+
ilya-biryukov wrote:
> Running `unparse` on an instance created via `URI::fromFile("")` will result
> in `"file:///"`.
>
> Have you considered returning a list of paths as a result from
> `switchHeaderSource`?
> That way you could capture the "no file matched" case with an empty list.
> Later when an implementation will look into index, that would allow to return
> multiple source files for a header file, which also happens in some C++
> projects.
>
> Returning an empty string is also an option we could start with.
I think I will start by returning an empty string for now. Returning a list of
paths sounds like a good idea once an indexer is implemented, but that would
require changing some parts of the code like find_if which returns only the
first instance of a match.
https://reviews.llvm.org/D36150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
Overall the code looks good, thanks for the clean-ups.
Only a few minor style comments and a major one about returning value when no
file is matching.
Please add tests for the new feature.
Comment at: clangd/ClangdLSPServer.cpp:227
+TextDocumentIdentifier Params, StringRef ID, JSONOutput ) {
+
+ llvm::Optional Result =
Maybe remove this empty line?
Comment at: clangd/ClangdLSPServer.cpp:231
+ URI ResultUri;
+ if (Result != llvm::None)
+ResultUri = URI::fromFile(*Result);
Replace with more concise form: `if (Result)`
Comment at: clangd/ClangdLSPServer.cpp:234
+ else
+ResultUri = URI::fromFile("");
+
Running `unparse` on an instance created via `URI::fromFile("")` will result
in `"file:///"`.
Have you considered returning a list of paths as a result from
`switchHeaderSource`?
That way you could capture the "no file matched" case with an empty list.
Later when an implementation will look into index, that would allow to return
multiple source files for a header file, which also happens in some C++
projects.
Returning an empty string is also an option we could start with.
Comment at: clangd/ClangdServer.cpp:22
+using namespace llvm;
using namespace clang;
Do we really need this `using namespace` or it can be deleted?
Comment at: clangd/ClangdServer.cpp:345
+if (FS->exists(NewPath)) {
+ return NewPath.str().str();
+}
Code style: we don't use braces around small single-statement control structures
Comment at: clangd/ClangdServer.h:185
+ /// Helper function that returns a path to the corresponding source file
when given a header file and vice versa.
+ llvm::Optional switchSourceHeader(PathRef Path);
Could you please `clang-format` this file too?
Comment line length is clearly over the limit.
https://reviews.llvm.org/D36150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
Nebiroth updated this revision to Diff 114424.
Nebiroth marked 7 inline comments as done.
Nebiroth added a comment.
Ran clang-format on modified files.
Minor refactoring.
https://reviews.llvm.org/D36150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
clangd/tool/ClangdMain.cpp
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -12,7 +12,6 @@
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Program.h"
-
#include
#include
#include
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -47,7 +47,9 @@
virtual void onCompletion(TextDocumentPositionParams Params, StringRef ID,
JSONOutput ) = 0;
virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
-JSONOutput ) = 0;
+JSONOutput ) = 0;
+ virtual void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
+JSONOutput ) = 0;
};
void regiterCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -204,6 +204,22 @@
ProtocolCallbacks
};
+struct SwitchSourceHeaderHandler : Handler {
+ SwitchSourceHeaderHandler(JSONOutput , ProtocolCallbacks )
+ : Handler(Output), Callbacks(Callbacks) {}
+
+ void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
+auto TDPP = TextDocumentIdentifier::parse(Params);
+if (!TDPP)
+ return;
+
+Callbacks.onSwitchSourceHeader(*TDPP, ID, Output);
+ }
+
+private:
+ ProtocolCallbacks
+};
+
} // namespace
void clangd::regiterCallbackHandlers(JSONRPCDispatcher ,
@@ -237,6 +253,10 @@
Dispatcher.registerHandler(
"textDocument/completion",
llvm::make_unique(Out, Callbacks));
- Dispatcher.registerHandler("textDocument/definition",
+ Dispatcher.registerHandler(
+ "textDocument/definition",
llvm::make_unique(Out, Callbacks));
+ Dispatcher.registerHandler(
+ "textDocument/switchSourceHeader",
+ llvm::make_unique(Out, Callbacks));
}
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -182,6 +182,9 @@
/// Get definition of symbol at a specified \p Line and \p Column in \p File.
Tagged findDefinitions(PathRef File, Position Pos);
+ /// Helper function that returns a path to the corresponding source file when given a header file and vice versa.
+ llvm::Optional switchSourceHeader(PathRef Path);
+
/// Run formatting for \p Rng inside \p File.
std::vector formatRange(PathRef File, Range Rng);
/// Run formatting for the whole \p File.
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -15,9 +15,11 @@
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/raw_ostream.h"
#include
+using namespace llvm;
using namespace clang;
using namespace clang::clangd;
@@ -273,16 +275,76 @@
return DumpFuture.get();
}
-Tagged
-ClangdServer::findDefinitions(PathRef File, Position Pos) {
+Tagged ClangdServer::findDefinitions(PathRef File,
+Position Pos) {
auto FileContents = DraftMgr.getDraft(File);
- assert(FileContents.Draft && "findDefinitions is called for non-added document");
+ assert(FileContents.Draft &&
+ "findDefinitions is called for non-added document");
std::vector Result;
auto TaggedFS = FSProvider.getTaggedFileSystem(File);
- Units.runOnUnit(File, *FileContents.Draft, ResourceDir, CDB, PCHs,
- TaggedFS.Value, [&](ClangdUnit ) {
-Result = Unit.findDefinitions(Pos);
- });
+ Units.runOnUnit(
+ File, *FileContents.Draft, ResourceDir, CDB, PCHs, TaggedFS.Value,
+ [&](ClangdUnit ) { Result = Unit.findDefinitions(Pos); });
return make_tagged(std::move(Result), TaggedFS.Tag);
}
+
+llvm::Optional ClangdServer::switchSourceHeader(PathRef Path) {
+
+ StringRef SourceExtensions[] = {".cpp", ".c", ".cc", ".cxx",
+ ".c++", ".m", ".mm"};
+ StringRef HeaderExtensions[] = {".h", ".hh", ".hpp", ".hxx", ".inc"};
+
+ StringRef PathExt = llvm::sys::path::extension(Path);
+
+ // Lookup in a list of known extensions.
+ auto SourceIter =
+
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
Just a few more comments. Should be easy to fix.
Could you also please `clang-format` the code?
Comment at: clangd/ClangdLSPServer.cpp:225
+ llvm::Optional Result =
LangServer.Server.switchSourceHeader(Params.uri.file);
+ Path ResultString = *Result;
+
What if `Result` does not contain a value?
Comment at: clangd/ClangdLSPServer.cpp:225
+ llvm::Optional Result =
LangServer.Server.switchSourceHeader(Params.uri.file);
+ Path ResultString = *Result;
+
ilya-biryukov wrote:
> What if `Result` does not contain a value?
Use `*Result` instead of introducing an extra variable?
Comment at: clangd/ClangdLSPServer.cpp:229
+R"({"jsonrpc":"2.0","id":)" + ID.str() +
+R"(,"result":)" + ResultString + R"(})");
+}
We should convert paths here. Convert to `URI` and use `unparse`.
```
Uri ResultUri = Uri::fromFile(*Result);
//
R"(,"result":)" + Uri::unparse(ResultUri) + R"(})");
//...
```
Comment at: clangd/ClangdServer.cpp:325
+ // Lookup in a list of known extensions.
+ auto SourceIter = std::find(std::begin(SourceExtensions),
std::end(SourceExtensions), PathExt);
+ bool IsSource = SourceIter != std::end(SourceExtensions);
Instead of an `checkUpperCaseExtensions` and two iterations use a single
`find_if` and `equals_lower`:
```
// Checks for both uppercase and lowercase matches in a single loop over
extensions array.
std::find_if(std::begin(SourceExtensions), std::end(SourceExtensions),
[](PathRef SourceExt) {
return SourceExt.equals_lower(PathExt);
});
```
Comment at: clangd/ClangdServer.cpp:363
+{
+ std::string FileHandle = "file://";
+ return "\"" + FileHandle + NewPath.str().str() + "\""; // First str() to
convert from SmallString to StringRef, second to convert from StringRef to
std::string
Don't add `"file://"` or quoting here, simply return `NewPath`.
Comment at: clangd/ClangdServer.h:186
+ /// If an extension hasn't been found in the lowercase array, try with
uppercase.
+ bool checkUpperCaseExtensions(StringRef BaseArray[], int ArraySize,
StringRef PathExt);
+
Any reason not to put it into anonymous namespace inside `.cpp` file?
Comment at: clangd/ProtocolHandlers.cpp:214
+if (!TDPP) {
+ return;
+}
Code style: we don't use braces around small single-statement control structures
https://reviews.llvm.org/D36150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
Nebiroth updated this revision to Diff 114054.
Nebiroth added a comment.
Some more code cleanup.
https://reviews.llvm.org/D36150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -48,6 +48,8 @@
JSONOutput ) = 0;
virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
JSONOutput ) = 0;
+ virtual void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
+JSONOutput ) = 0;
};
void regiterCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -204,6 +204,23 @@
ProtocolCallbacks
};
+struct SwitchSourceHeaderHandler : Handler {
+ SwitchSourceHeaderHandler(JSONOutput , ProtocolCallbacks )
+ : Handler(Output), Callbacks(Callbacks) {}
+
+ void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
+auto TDPP = TextDocumentIdentifier::parse(Params);
+if (!TDPP) {
+ return;
+}
+
+Callbacks.onSwitchSourceHeader(*TDPP, ID, Output);
+ }
+
+private:
+ ProtocolCallbacks
+};
+
} // namespace
void clangd::regiterCallbackHandlers(JSONRPCDispatcher ,
@@ -239,4 +256,7 @@
llvm::make_unique(Out, Callbacks));
Dispatcher.registerHandler("textDocument/definition",
llvm::make_unique(Out, Callbacks));
+ Dispatcher.registerHandler(
+ "textDocument/switchSourceHeader",
+ llvm::make_unique(Out, Callbacks));
}
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -182,6 +182,12 @@
/// Get definition of symbol at a specified \p Line and \p Column in \p File.
Tagged findDefinitions(PathRef File, Position Pos);
+ /// If an extension hasn't been found in the lowercase array, try with uppercase.
+ bool checkUpperCaseExtensions(StringRef BaseArray[], int ArraySize, StringRef PathExt);
+
+ /// Helper function that returns a path to the corresponding source file when given a header file and vice versa.
+ llvm::Optional switchSourceHeader(PathRef Path);
+
/// Run formatting for \p Rng inside \p File.
std::vector formatRange(PathRef File, Range Rng);
/// Run formatting for the whole \p File.
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -15,9 +15,11 @@
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/raw_ostream.h"
#include
+using namespace llvm;
using namespace clang;
using namespace clang::clangd;
@@ -286,3 +288,89 @@
});
return make_tagged(std::move(Result), TaggedFS.Tag);
}
+
+
+bool ClangdServer::checkUpperCaseExtensions(StringRef BaseArray[], int ArraySize, StringRef PathExt)
+{
+ std::string UpperExtensions[ArraySize];
+ for(int i = 0; i < ArraySize; i++)
+ {
+UpperExtensions[i] = BaseArray[i];
+ }
+ // Uppercase the whole array
+ for (std::string & s : UpperExtensions)
+ std::transform(s.begin(), s.end(), s.begin(),
+ [](unsigned char c) { return std::toupper(c); });
+
+ bool isUpperCase = false;
+ // Iterator such as std::begin won't work here so we use a standard loop
+ for(int i = 0; i < ArraySize; i++)
+ {
+if (UpperExtensions[i] == PathExt.str())
+{
+ isUpperCase = true;
+}
+ }
+ return isUpperCase;
+}
+
+llvm::Optional ClangdServer::switchSourceHeader(PathRef Path) {
+
+ StringRef SourceExtensions[] = {".cpp", ".c", ".cc", ".cxx", ".c++", ".m", ".mm"};
+ StringRef HeaderExtensions[] = {".h", ".hh", ".hpp", ".hxx", ".inc"};
+
+ StringRef PathExt = llvm::sys::path::extension(Path);
+
+ // Lookup in a list of known extensions.
+ auto SourceIter = std::find(std::begin(SourceExtensions), std::end(SourceExtensions), PathExt);
+ bool IsSource = SourceIter != std::end(SourceExtensions);
+
+ if (!IsSource)
+ {
+IsSource = checkUpperCaseExtensions(SourceExtensions, llvm::array_lengthof(SourceExtensions), PathExt);
+ }
+
+ auto HeaderIter = std::find(std::begin(HeaderExtensions), std::end(HeaderExtensions), PathExt);
+ bool IsHeader = HeaderIter != std::end(HeaderExtensions);
+
+ if (!IsHeader)
+ {
+IsHeader = checkUpperCaseExtensions(HeaderExtensions, llvm::array_lengthof(HeaderExtensions), PathExt);
+ }
+
+ // We can only switch between extensions known extensions.
+ if (!IsSource && !IsHeader)
+return llvm::None;
+
+ // Array to lookup extensions for the
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
Nebiroth updated this revision to Diff 114048.
Nebiroth added a comment.
Remove unintentional file addition
Updating D36150: [clangd] LSP extension to switch between source/header file
ll#
https://reviews.llvm.org/D36150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -48,6 +48,8 @@
JSONOutput ) = 0;
virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
JSONOutput ) = 0;
+ virtual void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
+JSONOutput ) = 0;
};
void regiterCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -204,6 +204,23 @@
ProtocolCallbacks
};
+struct SwitchSourceHeaderHandler : Handler {
+ SwitchSourceHeaderHandler(JSONOutput , ProtocolCallbacks )
+ : Handler(Output), Callbacks(Callbacks) {}
+
+ void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
+auto TDPP = TextDocumentIdentifier::parse(Params);
+if (!TDPP) {
+ return;
+}
+
+Callbacks.onSwitchSourceHeader(*TDPP, ID, Output);
+ }
+
+private:
+ ProtocolCallbacks
+};
+
} // namespace
void clangd::regiterCallbackHandlers(JSONRPCDispatcher ,
@@ -239,4 +256,7 @@
llvm::make_unique(Out, Callbacks));
Dispatcher.registerHandler("textDocument/definition",
llvm::make_unique(Out, Callbacks));
+ Dispatcher.registerHandler(
+ "textDocument/switchSourceHeader",
+ llvm::make_unique(Out, Callbacks));
}
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -182,6 +182,12 @@
/// Get definition of symbol at a specified \p Line and \p Column in \p File.
Tagged findDefinitions(PathRef File, Position Pos);
+ /// If an extension hasn't been found in the lowercase array, try with uppercase.
+ bool checkUpperCaseExtensions(StringRef BaseArray[], int ArraySize, StringRef PathExt);
+
+ /// Helper function that returns a path to the corresponding source file when given a header file and vice versa.
+ llvm::Optional switchSourceHeader(PathRef Path);
+
/// Run formatting for \p Rng inside \p File.
std::vector formatRange(PathRef File, Range Rng);
/// Run formatting for the whole \p File.
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -15,9 +15,11 @@
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/raw_ostream.h"
#include
+using namespace llvm;
using namespace clang;
using namespace clang::clangd;
@@ -286,3 +288,84 @@
});
return make_tagged(std::move(Result), TaggedFS.Tag);
}
+
+
+bool ClangdServer::checkUpperCaseExtensions(StringRef BaseArray[], int ArraySize, StringRef PathExt)
+{
+ std::string UpperExtensions[ArraySize];
+ for(int i = 0; i < ArraySize; i++)
+ {
+UpperExtensions[i] = BaseArray[i];
+ }
+ // Uppercase the whole array
+ for (std::string & s : UpperExtensions)
+ std::transform(s.begin(), s.end(), s.begin(),
+ [](unsigned char c) { return std::toupper(c); });
+
+ bool isUpperCase = false;
+ // Iterator such as std::begin won't work here so we use a standard loop
+ for(int i = 0; i < ArraySize; i++)
+ {
+if (UpperExtensions[i] == PathExt.str())
+{
+ isUpperCase = true;
+}
+ }
+ return isUpperCase;
+}
+
+llvm::Optional ClangdServer::switchSourceHeader(PathRef Path) {
+
+ StringRef SourceExtensions[] = {".cpp", ".c", ".cc", ".cxx", ".c++", ".m", ".mm"};
+ StringRef HeaderExtensions[] = {".h", ".hh", ".hpp", ".hxx", ".inc"};
+
+ StringRef PathExt = llvm::sys::path::extension(Path);
+
+ // Lookup in a list of known extensions.
+ auto SourceIter = std::find(std::begin(SourceExtensions), std::end(SourceExtensions), PathExt);
+ bool IsSource = SourceIter != std::end(SourceExtensions);
+
+ if (!IsSource)
+ {
+IsSource = checkUpperCaseExtensions(SourceExtensions, llvm::array_lengthof(SourceExtensions), PathExt);
+ }
+
+ auto HeaderIter = std::find(std::begin(HeaderExtensions), std::end(HeaderExtensions), PathExt);
+ bool IsHeader = HeaderIter != std::end(HeaderExtensions);
+
+ if (!IsHeader)
+ {
+IsHeader = checkUpperCaseExtensions(HeaderExtensions, llvm::array_lengthof(HeaderExtensions), PathExt);
+
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
Nebiroth updated this revision to Diff 114046.
Nebiroth added a comment.
Refactored switchSourceHeader function help from ilya
Added helper function to check for uppercase on current file.
https://reviews.llvm.org/D36150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
test/clangd/hover.test
Index: test/clangd/hover.test
===
--- /dev/null
+++ test/clangd/hover.test
@@ -0,0 +1,26 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 172
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"int main() {\nint a;\na;\n}\n"}}}
+
+Content-Length: 143
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":5}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":{"contents": {"language": "C++", "value": "int main() {\nint a;\na;\n}"}, "range": {"start": {"line": 0, "character": 0}, "end": {"line": 3, "character": 1
+
+Content-Length: 143
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":1,"character":5}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":{"contents": {"language": "C++", "value": "int a"}, "range": {"start": {"line": 1, "character": 0}, "end": {"line": 1, "character": 5
+
+Content-Length: 44
+
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -48,6 +48,8 @@
JSONOutput ) = 0;
virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
JSONOutput ) = 0;
+ virtual void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
+JSONOutput ) = 0;
};
void regiterCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -204,6 +204,23 @@
ProtocolCallbacks
};
+struct SwitchSourceHeaderHandler : Handler {
+ SwitchSourceHeaderHandler(JSONOutput , ProtocolCallbacks )
+ : Handler(Output), Callbacks(Callbacks) {}
+
+ void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
+auto TDPP = TextDocumentIdentifier::parse(Params);
+if (!TDPP) {
+ return;
+}
+
+Callbacks.onSwitchSourceHeader(*TDPP, ID, Output);
+ }
+
+private:
+ ProtocolCallbacks
+};
+
} // namespace
void clangd::regiterCallbackHandlers(JSONRPCDispatcher ,
@@ -239,4 +256,7 @@
llvm::make_unique(Out, Callbacks));
Dispatcher.registerHandler("textDocument/definition",
llvm::make_unique(Out, Callbacks));
+ Dispatcher.registerHandler(
+ "textDocument/switchSourceHeader",
+ llvm::make_unique(Out, Callbacks));
}
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -182,6 +182,12 @@
/// Get definition of symbol at a specified \p Line and \p Column in \p File.
Tagged findDefinitions(PathRef File, Position Pos);
+ /// If an extension hasn't been found in the lowercase array, try with uppercase.
+ bool checkUpperCaseExtensions(StringRef BaseArray[], int ArraySize, StringRef PathExt);
+
+ /// Helper function that returns a path to the corresponding source file when given a header file and vice versa.
+ llvm::Optional switchSourceHeader(PathRef Path);
+
/// Run formatting for \p Rng inside \p File.
std::vector formatRange(PathRef File, Range Rng);
/// Run formatting for the whole \p File.
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -15,9 +15,11 @@
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/raw_ostream.h"
#include
+using namespace llvm;
using namespace clang;
using namespace clang::clangd;
@@ -286,3 +288,84 @@
});
return make_tagged(std::move(Result), TaggedFS.Tag);
}
+
+
+bool ClangdServer::checkUpperCaseExtensions(StringRef BaseArray[], int ArraySize, StringRef PathExt)
+{
+ std::string UpperExtensions[ArraySize];
+ for(int i = 0; i < ArraySize; i++)
+ {
+
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
ilya-biryukov added a comment.
The function in `ClangdServer` seems to be getting more complicated and more
complicated, mostly because of mixing up `std::string`, `StringRef`,
`SmallString`.
Here's a slightly revamped implementation of your function, hope you'll find it
(or parts of it) useful to simplify your implementation and address remaining
comments.
// Note that the returned value is now llvm::Optional to clearly indicate no
matching file was found.
llvm::Optional ClangdServer::switchSourceHeader(PathRef Path) {
const StringRef SourceExtensions[] = {/*...*/; // only lower-case extensions
const StringRef HeaderExtensions[] = {/*...*/}; // ...
StringRef PathExt = llvm::sys::path::extension(Path);
// Lookup in a list of known extensions.
auto SourceIter = std::find(SourceExtensions, /*end iterator*/, PathExt,
[](StringRef LHS, StringRef RHS) { return LHS.equals_lower(RHS); });
bool IsSource = SourceIter != /*end iterator*/;
// ...
bool IsHeader = ...;
// We can only switch between extensions known extensions.
if (!IsSource && !IsHeader)
return llvm::None;
// Array to lookup extensions for the switch. An opposite of where original
extension was found.
ArrayRef NewExts = IsSource ? HeaderExtensions :
SourceExtensions;
// Storage for the new path.
SmallString<128> NewPath;
Path.toVector(NewPath);
// Instance of vfs::FileSystem, used for file existence checks.
auto FS = FSProvider.getTaggedFileSystem(Path).Value;
// Loop through switched extension candidates.
for (StringRef NewExt : NewExts) {
llvm::sys::path::replace_extension(NewPath, NewExt)
if (FS->exists(NewPath))
return NewPath.str().str(); // First str() to convert from SmallString
to StringRef, second to convert from StringRef to std::string
// Also check NewExt in upper-case, just in case.
llvm::sys::path::replace_extension(NewPath, NewExt.upper())
if (FS->exists(NewPath))
return NewPath.str().str();
}
return llvm::None;
}
Comment at: clangd/ClangdServer.cpp:404
+std::string returnValue = "file://" + std::string(test.str());
+returnValue = URI::unparse(URI::fromUri(returnValue));
+return Path(returnValue);
No need to create a `URI` here, this should be handled outside `ClangdServer`,
just return a path with replaced extension.
https://reviews.llvm.org/D36150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
arphaman added inline comments.
Comment at: clangd/ClangdServer.cpp:296
+
+ const int sourceSize = sizeof(DEFAULT_SOURCE_EXTENSIONS) /
sizeof(DEFAULT_SOURCE_EXTENSIONS[0]);
+ const int headerSize = sizeof(DEFAULT_HEADER_EXTENSIONS) /
sizeof(DEFAULT_HEADER_EXTENSIONS[0]);
You can use LLVM's function `array_lengthof` here and on the next line instead.
Comment at: clangd/ClangdServer.cpp:302
+ std::string *p;
+ p = std::find(DEFAULT_SOURCE_EXTENSIONS,
+DEFAULT_SOURCE_EXTENSIONS + sourceSize,
It might be better to use a `StringSet` instead of an array of strings and use
one lowercase/uppercase lookup:
```
llvm::StringSet<> DEFAULT_SOURCE_EXTENSIONS[] = {".cpp", ".c", ".cc", ".cxx",
".c++", ".m", ".mm"};
// Handles both lower and uppercase extensions and source/header files in one
if/else construct:
if (DEFAULT_SOURCE_EXTENSIONS.count(llvm::sys::path::extension(path).lower())) {
...
isSourceFile = true;
} else if
(DEFAULT_HEADER_EXTENSIONS.count(llvm::sys::path::extension(path).lower())) {
...
isSourceFile = false;
}
```
https://reviews.llvm.org/D36150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
Nebiroth updated this revision to Diff 111675.
Nebiroth added a comment.
Fixed more diff comments.
https://reviews.llvm.org/D36150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -48,6 +48,8 @@
JSONOutput ) = 0;
virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
JSONOutput ) = 0;
+ virtual void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
+JSONOutput ) = 0;
};
void regiterCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -204,6 +204,23 @@
ProtocolCallbacks
};
+struct SwitchSourceHeaderHandler : Handler {
+ SwitchSourceHeaderHandler(JSONOutput , ProtocolCallbacks )
+ : Handler(Output), Callbacks(Callbacks) {}
+
+ void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
+auto TDPP = TextDocumentIdentifier::parse(Params);
+if (!TDPP) {
+ return;
+}
+
+Callbacks.onSwitchSourceHeader(*TDPP, ID, Output);
+ }
+
+private:
+ ProtocolCallbacks
+};
+
} // namespace
void clangd::regiterCallbackHandlers(JSONRPCDispatcher ,
@@ -239,4 +256,7 @@
llvm::make_unique(Out, Callbacks));
Dispatcher.registerHandler("textDocument/definition",
llvm::make_unique(Out, Callbacks));
+ Dispatcher.registerHandler(
+ "textDocument/switchSourceHeader",
+ llvm::make_unique(Out, Callbacks));
}
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -182,6 +182,9 @@
/// Get definition of symbol at a specified \p Line and \p Column in \p File.
Tagged findDefinitions(PathRef File, Position Pos);
+ /// Helper function that returns a path to the corresponding source file when given a header file and vice versa.
+ Path switchSourceHeader(PathRef path);
+
/// Run formatting for \p Rng inside \p File.
std::vector formatRange(PathRef File, Range Rng);
/// Run formatting for the whole \p File.
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -15,6 +15,7 @@
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/raw_ostream.h"
#include
@@ -286,3 +287,127 @@
});
return make_tagged(std::move(Result), TaggedFS.Tag);
}
+
+Path ClangdServer::switchSourceHeader(PathRef path) {
+
+ std::string DEFAULT_SOURCE_EXTENSIONS[] = {".cpp", ".c", ".cc", ".cxx", ".c++", ".m", ".mm"};
+ std::string DEFAULT_HEADER_EXTENSIONS[] = {".h", ".hh", ".hpp", ".hxx", ".inc"};
+
+ const int sourceSize = sizeof(DEFAULT_SOURCE_EXTENSIONS) / sizeof(DEFAULT_SOURCE_EXTENSIONS[0]);
+ const int headerSize = sizeof(DEFAULT_HEADER_EXTENSIONS) / sizeof(DEFAULT_HEADER_EXTENSIONS[0]);
+
+ bool isSourceFile = false, foundExtension = false;
+ SmallString<128> NewPath;
+ std::string *p;
+ p = std::find(DEFAULT_SOURCE_EXTENSIONS,
+DEFAULT_SOURCE_EXTENSIONS + sourceSize,
+llvm::sys::path::extension(path));
+ if (p != DEFAULT_SOURCE_EXTENSIONS + sourceSize)
+ {
+std::string fileExtension = *p;
+NewPath = path;
+path = path.substr(0, (path.size() - fileExtension.size()));
+isSourceFile = true;
+foundExtension = true;
+ }
+ else
+ {
+std::string cpy[sourceSize];
+std::copy(DEFAULT_SOURCE_EXTENSIONS, DEFAULT_SOURCE_EXTENSIONS + sourceSize, cpy);
+for (std::string & s : cpy)
+std::transform(s.begin(), s.end(), s.begin(),
+ [](unsigned char c) { return std::toupper(c); });
+
+p = std::find(cpy, cpy + sourceSize, llvm::sys::path::extension(path));
+if (p != cpy + sourceSize)
+{
+std::string fileExtension = *p;
+NewPath = path;
+path = path.substr(0, (path.size() - fileExtension.size()));
+isSourceFile = true;
+foundExtension = true;
+}
+ }
+
+ if (!foundExtension) {
+p = std::find(DEFAULT_HEADER_EXTENSIONS,
+ DEFAULT_HEADER_EXTENSIONS + headerSize,
+ llvm::sys::path::extension(path));
+if (p != DEFAULT_HEADER_EXTENSIONS + headerSize)
+{
+ std::string fileExtension = *p;
+ NewPath = path;
+ path = path.substr(0, (path.size() - fileExtension.size()));
+ isSourceFile = false;
+ foundExtension = true;
+}
+else
+{
+ std::string cpy[sourceSize];
+
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.cpp:295
+ std::string DEFAULT_SOURCE_EXTENSIONS[] = { ".cpp", ".c", ".cc", ".cxx",
+".c++", ".C", ".m", ".mm" };
+ std::string DEFAULT_HEADER_EXTENSIONS[] = { ".h", ".hh", ".hpp", ".hxx",
ilya-biryukov wrote:
> We should check all extensions in both upper-case and lower-case, not just
> `.c` and `.C`
> (ideally, we could use case-insensitive comparisons).
It turns out there's a very simple way to compare case-insetively.
`StringRef(str).compare_lower(str2)`.
Could we use that instead of extension duplicates?
Comment at: clangd/ClangdServer.cpp:302
+
+ std::string pathDataRef = std::string(path.data());
+ bool isSourceFile = false, foundExtension = false;
ilya-biryukov wrote:
> `path` is already a `StringRef`, no need to convert it.
> `std::string` is also implicitly convertible to `StringRef`, you could pass
> `std::string` to every function that accepts a `StringRef`
I don't think this comment was addressed. Why do we need `pathDataRef`
variable?
https://reviews.llvm.org/D36150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
Nebiroth updated this revision to Diff 111368.
Nebiroth marked 8 inline comments as done.
Nebiroth added a comment.
Fixed diff comments.
https://reviews.llvm.org/D36150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -48,6 +48,8 @@
JSONOutput ) = 0;
virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
JSONOutput ) = 0;
+ virtual void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
+JSONOutput ) = 0;
};
void regiterCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -204,6 +204,23 @@
ProtocolCallbacks
};
+struct SwitchSourceHeaderHandler : Handler {
+ SwitchSourceHeaderHandler(JSONOutput , ProtocolCallbacks )
+ : Handler(Output), Callbacks(Callbacks) {}
+
+ void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
+auto TDPP = TextDocumentIdentifier::parse(Params);
+if (!TDPP) {
+ return;
+}
+
+Callbacks.onSwitchSourceHeader(*TDPP, ID, Output);
+ }
+
+private:
+ ProtocolCallbacks
+};
+
} // namespace
void clangd::regiterCallbackHandlers(JSONRPCDispatcher ,
@@ -239,4 +256,7 @@
llvm::make_unique(Out, Callbacks));
Dispatcher.registerHandler("textDocument/definition",
llvm::make_unique(Out, Callbacks));
+ Dispatcher.registerHandler(
+ "textDocument/switchSourceHeader",
+ llvm::make_unique(Out, Callbacks));
}
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -182,6 +182,9 @@
/// Get definition of symbol at a specified \p Line and \p Column in \p File.
Tagged findDefinitions(PathRef File, Position Pos);
+ /// Helper function that returns a path to the corresponding source file when given a header file and vice versa.
+ Path switchSourceHeader(PathRef path);
+
/// Run formatting for \p Rng inside \p File.
std::vector formatRange(PathRef File, Range Rng);
/// Run formatting for the whole \p File.
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -15,6 +15,7 @@
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/raw_ostream.h"
#include
@@ -286,3 +287,82 @@
});
return make_tagged(std::move(Result), TaggedFS.Tag);
}
+
+Path ClangdServer::switchSourceHeader(PathRef path) {
+
+ std::string DEFAULT_SOURCE_EXTENSIONS[] = {
+ ".cpp", ".c", ".cc", ".cxx", ".c++", ".m", ".mm",
+ ".CPP", ".C", ".CC", ".CXX", ".C++", ".M", ".MM"};
+ std::string DEFAULT_HEADER_EXTENSIONS[] = {
+ ".h", ".hh", ".hpp", ".hxx", ".inc", ".H", ".HH", ".HPP", ".HXX", ".INC"};
+
+ const int sourceSize =
+ sizeof(DEFAULT_SOURCE_EXTENSIONS) / sizeof(DEFAULT_SOURCE_EXTENSIONS[0]);
+ const int headerSize =
+ sizeof(DEFAULT_HEADER_EXTENSIONS) / sizeof(DEFAULT_HEADER_EXTENSIONS[0]);
+
+ std::string pathDataRef = std::string(path);
+ bool isSourceFile = false, foundExtension = false;
+ SmallString<128> NewPath;
+ std::string *p;
+ p = std::find(DEFAULT_SOURCE_EXTENSIONS,
+DEFAULT_SOURCE_EXTENSIONS + sourceSize,
+llvm::sys::path::extension(path));
+ if (p != DEFAULT_SOURCE_EXTENSIONS + sourceSize)
+ {
+std::string fileExtension = *p;
+NewPath = pathDataRef;
+pathDataRef =
+pathDataRef.substr(0, (pathDataRef.size() - fileExtension.size()));
+isSourceFile = true;
+foundExtension = true;
+ }
+
+ if (!foundExtension) {
+p = std::find(DEFAULT_HEADER_EXTENSIONS,
+ DEFAULT_HEADER_EXTENSIONS + headerSize,
+ llvm::sys::path::extension(path));
+if (p != DEFAULT_HEADER_EXTENSIONS + headerSize) {
+ std::string fileExtension = *p;
+ NewPath = pathDataRef;
+ pathDataRef =
+ pathDataRef.substr(0, (pathDataRef.size() - fileExtension.size()));
+ isSourceFile = false;
+ foundExtension = true;
+}
+ }
+
+ SmallString<128> CurrentPath;
+ CurrentPath = std::string(pathDataRef);
+ bool done = false;
+ ArrayRef ExtensionsArray;
+
+ if (!isSourceFile)
+ExtensionsArray = DEFAULT_SOURCE_EXTENSIONS;
+ else
+ExtensionsArray = DEFAULT_HEADER_EXTENSIONS;
+
+ int i = 0;
+ while (!done && i < ExtensionsArray.size()) {
+std::string CurrentPathString = CurrentPath.str();
+
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
malaperle added inline comments. Comment at: clangd/ProtocolHandlers.cpp:260 + Dispatcher.registerHandler( + "textDocument/switchSourceHeader", + llvm::make_unique(Out, Callbacks)); ilya-biryukov wrote: > ilya-biryukov wrote: > > I don't see this method in the [[ > > https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md > > | LSP spec ]]. > > Is this some kind of extension? > > > > Could you add some comments on that with pointers to proposal/explanation > > of where this extension is used? > This comment was not addressed, probably marked as 'Done' by accident. > Is this some kind of extension? It's briefly mentioned in the summary but could use some more detail. The first client to use it is Theia but I wonder if it would be easy to add it to the VS Code extension, as an example. https://reviews.llvm.org/D36150 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
This is probably not a final version, but I feel my comments should be helpful
anyway.
Also, we're missing testcases now.
Comment at: clangd/ClangdLSPServer.cpp:225
+ PathRef ref = PathRef((Params.uri.uri).c_str());
+ Path result = LangServer.Server.switchSourceHeader(ref);
+
We use `Params.uri.file`, not `Params.uri.uri` when passing paths to clangd.
Also no need for extra local var:
`LangServer.Server.switchSourceHeader(Params.uri.file);` should work.
Comment at: clangd/ClangdLSPServer.cpp:225
+ PathRef ref = PathRef((Params.uri.uri).c_str());
+ Path result = LangServer.Server.switchSourceHeader(ref);
+
ilya-biryukov wrote:
> We use `Params.uri.file`, not `Params.uri.uri` when passing paths to clangd.
> Also no need for extra local var:
> `LangServer.Server.switchSourceHeader(Params.uri.file);` should work.
Naming: local vars should start with capital letter.
Comment at: clangd/ClangdServer.cpp:295
+ std::string DEFAULT_SOURCE_EXTENSIONS[] = { ".cpp", ".c", ".cc", ".cxx",
+".c++", ".C", ".m", ".mm" };
+ std::string DEFAULT_HEADER_EXTENSIONS[] = { ".h", ".hh", ".hpp", ".hxx",
We should check all extensions in both upper-case and lower-case, not just `.c`
and `.C`
(ideally, we could use case-insensitive comparisons).
Comment at: clangd/ClangdServer.cpp:302
+
+ std::string pathDataRef = std::string(path.data());
+ bool isSourceFile = false, foundExtension = false;
`path` is already a `StringRef`, no need to convert it.
`std::string` is also implicitly convertible to `StringRef`, you could pass
`std::string` to every function that accepts a `StringRef`
Comment at: clangd/ClangdServer.cpp:307
+
+ while (!foundExtension && i < sourceSize)
+ {
Maybe replace with:
```
if (std::find(DEFAULT_SOURCE_EXTESIONS, /*end iterator*/,
llvm::sys::path::extensions(path)) {
isSourceFile = true;
foundExtension = true;
}
```
Comment at: clangd/ClangdServer.cpp:323
+ {
+while (!foundExtension && i < headerSize)
+{
Maybe use `std::find` here too?
Comment at: clangd/ClangdServer.cpp:339
+ SmallString<128> CurrentPath;
+ CurrentPath = temp;
+ bool done = false;
Don't need `temp` var here, there's `StringRef::toVector` method to convert
`StringRef` to `SmallString`.
Comment at: clangd/ClangdServer.cpp:341
+ bool done = false;
+ std::vector EXTENSIONS_ARRAY;
+
Naming: EXTENSIONS_ARRAY is a local var, use `UpperCamelCase`.
Maybe use `ArrayRef` for `ExtensionsArray` instead?
Comment at: clangd/ClangdServer.cpp:360
+FILE * file;
+file = fopen(temp.c_str(), "r");
+if (file)
Please use `vfs::FileSystem` instance, provided by
`FSProvider.getTaggedFileSystem()` here for file accesses.
Comment at: clangd/ClangdServer.cpp:367
+ std::string returnValue = std::string(NewPath.str());
+ return Path("\"" + returnValue + "\"");
+}
Don't add quotes here, `Uri` constructor and `Uri::unparse` is responsible for
doing that.
Comment at: clangd/ClangdServer.h:185
+ std::string switchSourceHeader(std::string path);
+
ilya-biryukov wrote:
> ilya-biryukov wrote:
> > Please use descriptive typedefs for paths: `Path` (equivalent to
> > `std::string`) for return type and `PathRef` (equivalent to `StringRef`)
> > for parameter type.
> Maybe add a small comment for this function to indicate it's a fairly trivial
> helper?
Maybe also change wording to indicate that this function does not mutate any
state and is indeed trivial, i.e.:
`Helper function that returns a path to the corresponding source file when
given a header file and vice versa.`
Code style: please end all comments with full stops.
Comment at: clangd/ClangdServer.h:186
+ // Helper function to change from one header file to it's corresponding
source file and vice versa
+ Path switchSourceHeader(PathRef path);
+
Naming: parameter names must be capitalized
Comment at: clangd/ProtocolHandlers.cpp:260
+ Dispatcher.registerHandler(
+ "textDocument/switchSourceHeader",
+ llvm::make_unique(Out, Callbacks));
ilya-biryukov wrote:
> I don't see this method in the [[
> https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md
> | LSP spec ]].
> Is this some kind of extension?
>
> Could you add some comments on that with pointers to proposal/explanation of
> where this extension is used?
This comment was not addressed, probably marked as 'Done' by accident.
https://reviews.llvm.org/D36150
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
Nebiroth updated this revision to Diff 109566.
Nebiroth marked 7 inline comments as done.
Nebiroth added a comment.
[clangd] LSP extension to switch between source/header file
https://reviews.llvm.org/D36150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -48,6 +48,8 @@
JSONOutput ) = 0;
virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
JSONOutput ) = 0;
+ virtual void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
+JSONOutput ) = 0;
};
void regiterCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -204,6 +204,23 @@
ProtocolCallbacks
};
+struct SwitchSourceHeaderHandler : Handler {
+ SwitchSourceHeaderHandler(JSONOutput , ProtocolCallbacks )
+ : Handler(Output), Callbacks(Callbacks) {}
+
+ void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
+auto TDPP = TextDocumentIdentifier::parse(Params);
+if (!TDPP) {
+ return;
+}
+
+Callbacks.onSwitchSourceHeader(*TDPP, ID, Output);
+ }
+
+private:
+ ProtocolCallbacks
+};
+
} // namespace
void clangd::regiterCallbackHandlers(JSONRPCDispatcher ,
@@ -239,4 +256,7 @@
llvm::make_unique(Out, Callbacks));
Dispatcher.registerHandler("textDocument/definition",
llvm::make_unique(Out, Callbacks));
+ Dispatcher.registerHandler(
+ "textDocument/switchSourceHeader",
+ llvm::make_unique(Out, Callbacks));
}
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -182,6 +182,9 @@
/// Get definition of symbol at a specified \p Line and \p Column in \p File.
Tagged findDefinitions(PathRef File, Position Pos);
+ // Helper function to change from one header file to it's corresponding source file and vice versa
+ Path switchSourceHeader(PathRef path);
+
/// Run formatting for \p Rng inside \p File.
std::vector formatRange(PathRef File, Range Rng);
/// Run formatting for the whole \p File.
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -16,13 +16,15 @@
#include "llvm/ADT/ArrayRef.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/raw_ostream.h"
+#include "llvm/Support/Path.h"
#include
using namespace clang;
using namespace clang::clangd;
namespace {
+
std::vector formatCode(StringRef Code, StringRef Filename,
ArrayRef Ranges) {
// Call clang-format.
@@ -286,3 +288,86 @@
});
return make_tagged(std::move(Result), TaggedFS.Tag);
}
+
+Path ClangdServer::switchSourceHeader(PathRef path) {
+
+ std::string DEFAULT_SOURCE_EXTENSIONS[] = { ".cpp", ".c", ".cc", ".cxx",
+".c++", ".C", ".m", ".mm" };
+ std::string DEFAULT_HEADER_EXTENSIONS[] = { ".h", ".hh", ".hpp", ".hxx",
+".inc" };
+
+ const int sourceSize = sizeof(DEFAULT_SOURCE_EXTENSIONS)/sizeof(DEFAULT_SOURCE_EXTENSIONS[0]);
+ const int headerSize = sizeof(DEFAULT_HEADER_EXTENSIONS)/sizeof(DEFAULT_HEADER_EXTENSIONS[0]);
+
+ std::string pathDataRef = std::string(path.data());
+ bool isSourceFile = false, foundExtension = false;
+ SmallString<128> NewPath;
+ int i = 0;
+
+ while (!foundExtension && i < sourceSize)
+ {
+if (llvm::sys::path::extension(pathDataRef) == DEFAULT_SOURCE_EXTENSIONS[i])
+{
+ std::string fileExtension = std::string(DEFAULT_SOURCE_EXTENSIONS[i]);
+ NewPath = pathDataRef;
+ pathDataRef = pathDataRef.substr(0, (pathDataRef.length() - fileExtension.size()));
+ isSourceFile = true;
+ foundExtension = true;
+}
+i++;
+ }
+ i = 0;
+
+ if (!foundExtension)
+ {
+while (!foundExtension && i < headerSize)
+{
+ if (llvm::sys::path::extension(pathDataRef) == DEFAULT_HEADER_EXTENSIONS[i])
+ {
+std::string fileExtension = std::string(DEFAULT_HEADER_EXTENSIONS[i]);
+NewPath = pathDataRef;
+pathDataRef = pathDataRef.substr(0, (pathDataRef.length() - fileExtension.size()));
+isSourceFile = false;
+foundExtension = true;
+ }
+i++;
+}
+ }
+
+ std::string temp = std::string(pathDataRef);
+ SmallString<128> CurrentPath;
+ CurrentPath = temp;
+ bool done = false;
+ std::vector EXTENSIONS_ARRAY;
+
+ if (!isSourceFile)
+EXTENSIONS_ARRAY.assign(DEFAULT_SOURCE_EXTENSIONS, DEFAULT_SOURCE_EXTENSIONS + sourceSize);
+
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
ilya-biryukov requested changes to this revision.
ilya-biryukov added a reviewer: ilya-biryukov.
ilya-biryukov added a comment.
Also, +1 to the comments about file extensions, we have to cover at least `.c`,
`.cc` and `.cpp` for source files, `.h` and `.hpp` for header files.
Comment at: clangd/ClangdLSPServer.cpp:228
+R"({"jsonrpc":"2.0","id":)" + ID.str() +
+R"(,"result":)" + result + R"(})");
+}
We should probably use `Uri` here.
Comment at: clangd/ClangdServer.h:185
+ std::string switchSourceHeader(std::string path);
+
Please use descriptive typedefs for paths: `Path` (equivalent to `std::string`)
for return type and `PathRef` (equivalent to `StringRef`) for parameter type.
Comment at: clangd/ClangdServer.h:185
+ std::string switchSourceHeader(std::string path);
+
ilya-biryukov wrote:
> Please use descriptive typedefs for paths: `Path` (equivalent to
> `std::string`) for return type and `PathRef` (equivalent to `StringRef`) for
> parameter type.
Maybe add a small comment for this function to indicate it's a fairly trivial
helper?
Comment at: clangd/ProtocolHandlers.cpp:260
+ Dispatcher.registerHandler(
+ "textDocument/switchSourceHeader",
+ llvm::make_unique(Out, Callbacks));
I don't see this method in the [[
https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md |
LSP spec ]].
Is this some kind of extension?
Could you add some comments on that with pointers to proposal/explanation of
where this extension is used?
https://reviews.llvm.org/D36150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
arphaman added inline comments.
Comment at: clangd/ClangdServer.cpp:292
+
+ if (path.compare(path.length() - 4, 4, ".cpp") == 0) {
+path = path.substr(0, (path.length() - 4));
malaperle wrote:
> this won't work for other extensions, we need to make this more generic. I
> believe you had a list of typical source files extensions and header
> extensions before?
LLVM has a wonderful `sys::path` library for path manipulations. You could
rewrite these checks like this:
```
if (llvm::sys::path::extension(path) == ".cpp") {
SmallString<128> NewPath = path;
llvm::sys::path::replace_extension(NewPath, "h");
return "\"" + NewPath.str() + "\"";
}
```
https://reviews.llvm.org/D36150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
malaperle added a comment. In the commit message, you could add that we will use and index of symbols later to be able to switch from header to source file when the file names don't match. This is more or less the justification of why we want this in Clangd and not on the client. https://reviews.llvm.org/D36150 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.
Comment at: clangd/ClangdServer.cpp:292
+
+ if (path.compare(path.length() - 4, 4, ".cpp") == 0) {
+path = path.substr(0, (path.length() - 4));
this won't work for other extensions, we need to make this more generic. I
believe you had a list of typical source files extensions and header extensions
before?
Comment at: clangd/ClangdServer.cpp:294
+path = path.substr(0, (path.length() - 4));
+path.append(".h");
+return "\"" + path + "\"";
we need to try if the file exists otherwise try other typical header extensions
Comment at: clangd/ClangdServer.cpp:296
+return "\"" + path + "\"";
+ } else if (path.compare(path.length() - 2, 2, ".h") == 0) {
+path = path.substr(0, (path.length() - 2));
needs to be more generic and handle more typical header extensions
Comment at: test/clangd/hover.test:1
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
this is from another patch
https://reviews.llvm.org/D36150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36150: [clangd] LSP extension to switch between source/header file
Nebiroth created this revision.
Small extension to LSP to allow clients to use clangd to switch between C
header files and source files.
Final version will use the completed clangd indexer.
https://reviews.llvm.org/D36150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
test/clangd/hover.test
Index: test/clangd/hover.test
===
--- /dev/null
+++ test/clangd/hover.test
@@ -0,0 +1,26 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 172
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"int main() {\nint a;\na;\n}\n"}}}
+
+Content-Length: 143
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":5}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":{"contents": {"language": "C++", "value": "int main() {\nint a;\na;\n}"}, "range": {"start": {"line": 0, "character": 0}, "end": {"line": 3, "character": 1
+
+Content-Length: 143
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":1,"character":5}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":{"contents": {"language": "C++", "value": "int a"}, "range": {"start": {"line": 1, "character": 0}, "end": {"line": 1, "character": 5
+
+Content-Length: 44
+
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -48,6 +48,8 @@
JSONOutput ) = 0;
virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
JSONOutput ) = 0;
+ virtual void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
+JSONOutput ) = 0;
};
void regiterCallbackHandlers(JSONRPCDispatcher , JSONOutput ,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -204,6 +204,23 @@
ProtocolCallbacks
};
+struct SwitchSourceHeaderHandler : Handler {
+ SwitchSourceHeaderHandler(JSONOutput , ProtocolCallbacks )
+ : Handler(Output), Callbacks(Callbacks) {}
+
+ void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
+auto TDPP = TextDocumentIdentifier::parse(Params);
+if (!TDPP) {
+ return;
+}
+
+Callbacks.onSwitchSourceHeader(*TDPP, ID, Output);
+ }
+
+private:
+ ProtocolCallbacks
+};
+
} // namespace
void clangd::regiterCallbackHandlers(JSONRPCDispatcher ,
@@ -239,4 +256,7 @@
llvm::make_unique(Out, Callbacks));
Dispatcher.registerHandler("textDocument/definition",
llvm::make_unique(Out, Callbacks));
+ Dispatcher.registerHandler(
+ "textDocument/switchSourceHeader",
+ llvm::make_unique(Out, Callbacks));
}
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -182,6 +182,8 @@
/// Get definition of symbol at a specified \p Line and \p Column in \p File.
Tagged findDefinitions(PathRef File, Position Pos);
+ std::string switchSourceHeader(std::string path);
+
/// Run formatting for \p Rng inside \p File.
std::vector formatRange(PathRef File, Range Rng);
/// Run formatting for the whole \p File.
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -286,3 +286,18 @@
});
return make_tagged(std::move(Result), TaggedFS.Tag);
}
+
+std::string ClangdServer::switchSourceHeader(std::string path) {
+
+ if (path.compare(path.length() - 4, 4, ".cpp") == 0) {
+path = path.substr(0, (path.length() - 4));
+path.append(".h");
+return "\"" + path + "\"";
+ } else if (path.compare(path.length() - 2, 2, ".h") == 0) {
+path = path.substr(0, (path.length() - 2));
+path.append(".cpp");
+return "\"" + path + "\"";
+ } else {
+return "";
+ }
+}
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -71,6 +71,8 @@
JSONOutput ) override;
void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
JSONOutput ) override;
+ void
