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
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
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
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
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
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
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 =
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
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
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
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
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
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
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
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
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
+
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:
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#
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
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)
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]);
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:
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",
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
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
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:
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
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.
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
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.
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));
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
32 matches
Mail list logo
