ilya-biryukov added a comment.
@francisco.lopes, I've surely noticed there are a bunch of places where
`signatureHelp` does not show up while I was playing around with this change.
It would be great to have it available in more cases.
Repository:
rL LLVM
https://reviews.llvm.org/D38048
__
This revision was automatically updated to reflect the committed changes.
Closed by commit rL315055: [clangd] Add textDocument/signatureHelp (authored by
ibiryukov).
Changed prior to commit:
https://reviews.llvm.org/D38048?vs=117580&id=117983#toc
Repository:
rL LLVM
https://reviews.llvm.org
francisco.lopes added a comment.
In https://reviews.llvm.org/D38048#889568, @rwols wrote:
> Since we're throwing around gifs for fun here's how signatureHelp looks in
> practice for Sublime Text
Very cool!
https://reviews.llvm.org/D38048
___
cfe
rwols added a comment.
In https://reviews.llvm.org/D38048#887960, @ilya-biryukov wrote:
> LGTM. Do you need my help in landing this?
Yes, I don't have commit access, go ahead and merge. If you find any merge
conflicts let me know I can probably fix that.
Since we're throwing around gifs for fu
francisco.lopes added a comment.
In https://reviews.llvm.org/D38048#889176, @ilya-biryukov wrote:
> @francisco.lopes, thanks for sharing your experience, I definitely like the
> final UX you achieved, we should aim for something similar in clangd.
> It also looks like @rwols's idea of how thin
ilya-biryukov added a comment.
@francisco.lopes, thanks for sharing your experience, I definitely like the
final UX you achieved, we should aim for something similar in clangd.
It also looks like @rwols's idea of how things should look like is very close
to your implementation.
I do think that
francisco.lopes added a comment.
In https://reviews.llvm.org/D38048#887960, @ilya-biryukov wrote:
> Another case where this might be bad is overloaded functions. I may choose
> one overload in completion, but `signatureHelp` will initially point into a
> different one.
Hi, I'd just like to c
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM. Do you need my help in landing this?
> 1. If it finds a callable, provide the name of the callable, followed by an
> opening parenthesis, followed by `$0`, followed by a cl
rwols marked an inline comment as done.
rwols added a comment.
Thinking ahead, it's probably best if we change the behavior of
`SnippetCompletionItemCollector` as follows:
1. If it finds a callable, provide the name of the callable, followed by an
opening parenthesis, followed by `$0`, followed
rwols updated this revision to Diff 117580.
rwols added a comment.
Reword the documentation of the signature help structures in Protocol.h
https://reviews.llvm.org/D38048
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Cl
malaperle added inline comments.
Comment at: clangd/Protocol.h:458
+///
+/// A parameter can have a label and a doc-comment.
+struct ParameterInformation {
rwols wrote:
> @malaperle I copied the sentences from the protocol markdown file over
> [here](https://git
rwols updated this revision to Diff 117427.
rwols added a comment.
- Update `invokeCodeComplete` function to also take a `CodeCompleteOptions`, we
assign it to the `CompilerInstance`'s `FrontendOpts.CodeCompleteOpts`.
- Remove unused variable in `SignatureHelpCollector::ProcessOverloadCandidate`.
ilya-biryukov added inline comments.
Comment at: clangd/ClangdUnit.cpp:742
+ Consumer->includeBriefComments();
+ FrontendOpts.CodeCompleteOpts.IncludeBriefComments =
+ Consumer->includeBriefComments();
Duplicated line sneaked into commit. It looks like
rwols updated this revision to Diff 117290.
rwols added a comment.
- Fix fillSortText: used the old version, now using new version from upstream
- Fix initialize-params.test and initialize-params-invalid.test to account for
signature help
- All tests pass now
https://reviews.llvm.org/D38048
Fi
rwols added inline comments.
Comment at: clangd/ClangdUnit.cpp:442
static void FillSortText(const CodeCompletionString &CCS,
CompletionItem &Item) {
I seem to have inadvertently used the old fillSortText during the merge from
ups
rwols added inline comments.
Comment at: clangd/Protocol.h:458
+///
+/// A parameter can have a label and a doc-comment.
+struct ParameterInformation {
@malaperle I copied the sentences from the protocol markdown file over
[here](https://github.com/Microsoft/lan
rwols marked 2 inline comments as done.
rwols added a comment.
I see now I have accidentally removed that new sortText logic during the merge
from upstream, let me fix that.
https://reviews.llvm.org/D38048
___
cfe-commits mailing list
cfe-commits@l
rwols marked 4 inline comments as done.
rwols added a comment.
There were some failing tests, probably because we use an extra digit for the
`sortText` property now. I haven't touched those tests.
Comment at: clangd/ClangdUnit.cpp:610
+ParameterInformation Info;
+
rwols updated this revision to Diff 117261.
rwols added a comment.
- Updated to latest upstream revision.
- Fix optional/default parameter handling: each default parameter now has its
own label (thanks, ilya!).
- Remove the templated "invokeClangAction" function and use polymorphism with
CodeCom
ilya-biryukov added a comment.
Sorry for late response.
Comment at: clangd/ClangdUnit.cpp:610
+ParameterInformation Info;
+OptionalParameterLabel = getOptionalString(*Chunk.Optional);
+Result.label += OptionalParameterLabel;
Are we first
rwols marked 3 inline comments as done.
rwols added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:92
"completionProvider": {"resolveProvider": false,
"triggerCharacters": [".",">",":"]},
+ "signatureHelpProvider": {"triggerCharacters": ["(", ","]},
rwols updated this revision to Diff 116466.
rwols added a comment.
- Add signature-help.test
- Modify formatting.test to make the test pass. The test failed because
clangd's response to the initialize request was changed to advertise signature
help trigger characters, so the expected content len
rwols updated this revision to Diff 116461.
rwols added a comment.
- Update doxygen comment to say "signature help" instead of "code complete",
- Refactor: put the FillDocumentation member function outside of
`CompletionItemsCollector` so that `SignatureHelpCollector` can also use it.
- Refactor:
ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
Thanks for the patch. Overall looks good, just a few comments.
Could you also add some tests?
Comment at: clangd/ClangdServer.h:243
+ /// Provide sig
rwols created this revision.
Makes clangd respond to a client's "textDocument/signatureHelp" request by
presenting function/method overloads.
Still need to add tests.
Also, there is duplicate code in clangd::codeComplete and clangd::signatureHelp
now, maybe refactor this to a common function?
ht
25 matches
Mail list logo
