[PATCH] D38083: [clangd] Skip informative qualifier chunks.

rwols added inline comments. Comment at: test/clangd/completion-qualifiers.test:12 +# CHECK: {"jsonrpc":"2.0","id":2,"result":[ +# CHEKC-DAG: {"label":"foo() const","kind":2,"detail":"int","sortText":"00035foo","filterText":"foo","insertText":"foo","insertTextFormat":1} +#

[PATCH] D38048: [clangd] Add textDocument/signatureHelp

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?

[PATCH] D38048: [clangd] Add textDocument/signatureHelp

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

[PATCH] D38048: [clangd] Add textDocument/signatureHelp

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`,

[PATCH] D38048: [clangd] Add textDocument/signatureHelp

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

[PATCH] D38939: [clangd] Handle exit notification (proper shutdown)

rwols created this revision. This changes the onShutdown handler to do essentially nothing (for now), and instead exits the runloop when we receive the exit notification from the client. Some clients may wait on the reply from the shutdown request before sending an exit notification. If we exit

[PATCH] D38939: [clangd] Handle exit notification (proper shutdown)

rwols added inline comments. Comment at: test/clangd/authority-less-uri.test:33 {"jsonrpc":"2.0","id":3,"method":"shutdown"} +# CHECK: {"jsonrpc":"2.0","id":3,"result":null} +Content-Length: 33 sammccall wrote: > Having the shutdown/exit boilerplate in every

[PATCH] D38939: [clangd] Handle exit notification (proper shutdown)

rwols added a comment. I couldn't get the protocol.test to succeed; `clangd` returns 1 now and that trips up `lit`. I think I have to use `XFAIL` from `lit` somehow but didn't figure it out. https://reviews.llvm.org/D38939 ___ cfe-commits mailing

[PATCH] D38939: [clangd] Handle exit notification (proper shutdown)

rwols updated this revision to Diff 119198. rwols added a comment. Return 0 if shutdown was received before exit, otherwise return 1 The protocol.test test fails now. https://reviews.llvm.org/D38939 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/Protocol.h

[PATCH] D37101: [clangd] Add support for snippet completions

rwols updated this revision to Diff 114415. rwols added a comment. Update the description for the "-enable-snippets" option. https://reviews.llvm.org/D37101 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/ClangdUnit.cpp

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

rwols marked 10 inline comments as done. rwols added a comment. I followed your advice and kept the snippet functionality. We'll do the SignatureHelp stuff in another review. A "major" change is that, because CK_Informative chunks are put into the label now, we have to use the insertText,

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

rwols updated this revision to Diff 113219. rwols added a comment. Tidy up snippet handling - Put CK_Informative chunks in the label - Assert that there's at most one CK_ResultType chunk - CK_CurrentParameter never occurs while collecting completions, only while handling overloads. - For

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

rwols added a comment. So at this point the C++ changes are basically done, save for some minor things I guess. The problem is still that the VSCode extension doesn't do anything clever with the snippets. I have zero experience with TypeScript let alone extension development for VSCode, so it

[PATCH] D37101: [clangd] Add support for snippet completions

rwols added a comment. No, I don't have access to the repo, sorry forgot to mention this. https://reviews.llvm.org/D37101 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37101: [clangd] Add support for snippet completions

rwols marked 7 inline comments as done. rwols added inline comments. Comment at: clangd/ClangdUnit.cpp:376 + +CompletionItem Item{InsertTextFormat::PlainText}; + ilya-biryukov wrote: > Implementations of this function in `PlainTextCompletionItemsCollector`

[PATCH] D37101: [clangd] Add support for snippet completions

rwols updated this revision to Diff 113901. rwols added a comment. - Fix failing clangd tests https://reviews.llvm.org/D37101 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/ClangdUnit.cpp clangd/ClangdUnit.h

[PATCH] D37101: [clangd] Add support for snippet completions

rwols updated this revision to Diff 113899. rwols added a comment. - Don't use const for non-reference parameters - `lowerCamelCase` for functions, verb phrase - Field definitions at the bottom of the class - Make ProcessChunks virtual - Add comment that fallthrough to the next case is intended -

[PATCH] D37101: [clangd] Add support for snippet completions

rwols marked an inline comment as done. rwols added inline comments. Comment at: clangd/tool/ClangdMain.cpp:33 + "present plaintext completions."), +llvm::cl::init(false)); + ilya-biryukov wrote: > After

[PATCH] D37101: [clangd] Add support for snippet completions

rwols updated this revision to Diff 114242. rwols added a comment. Change command-line option back to "-enable-snippets", disable snippets by default. This is a temporary solution and we should instead inspect the "initialize" request from the client to check wether the client supports

[PATCH] D37101: [clangd] Add support for snippet completions

rwols added a comment. Thanks! Thinking ahead now so we're on the same page, you will be implementing the client's initialize request, and I'll start work on textDocument/signatureHelp. https://reviews.llvm.org/D37101 ___ cfe-commits mailing list

[PATCH] D38048: [clangd] Add textDocument/signatureHelp

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; +

[PATCH] D38048: [clangd] Add textDocument/signatureHelp

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

[PATCH] D38048: [clangd] Add textDocument/signatureHelp

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

[PATCH] D38048: [clangd] Add textDocument/signatureHelp

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

[PATCH] D38048: [clangd] Add textDocument/signatureHelp

rwols added inline comments. Comment at: clangd/ClangdUnit.cpp:442 static void FillSortText(const CodeCompletionString , CompletionItem ) { I seem to have inadvertently used the old fillSortText during the merge from upstream,

[PATCH] D38048: [clangd] Add textDocument/signatureHelp

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

[PATCH] D38225: [clangd] Fix missing "message" key when responding with unsupported method

rwols added a comment. No, I don't have commit access. Feel free to merge. https://reviews.llvm.org/D38225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38048: [clangd] Add textDocument/signatureHelp

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. -

[PATCH] D38048: [clangd] Add textDocument/signatureHelp

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

[PATCH] D38083: [clangd] Skip informative qualifier chunks.

rwols accepted this revision. rwols added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D38083 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D38048: [clangd] Add textDocument/signatureHelp

rwols marked 3 inline comments as done. rwols added inline comments. Comment at: clangd/ClangdLSPServer.cpp:92 "completionProvider": {"resolveProvider": false, "triggerCharacters": [".",">",":"]}, + "signatureHelpProvider": {"triggerCharacters": ["(", ","]},

[PATCH] D38225: [clangd] Fix missing "message" key when responding with unsupported method

rwols created this revision. The language server protocol dictates that a ResponseError should have a [message string][1] describing the error. This adds a simple message to the error and a simple test. [1]:

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

rwols added a comment. Thanks for the quick review! I'm new to Phabricator and the `arc` CLI tool. Is the workflow like this: "address a comment, change a few lines, do `arc diff`, do this multiple times", or is it like this: "address all the comments, change lots of lines, do `arc diff`, do

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

rwols updated this revision to Diff 112609. rwols added a comment. [clangd] [WIP] Add support for snippet completions - Restore the sortText logic - Return CompletionItem instead of modifying a ref param - Make all helper methods const - Only set to Snippet once we encounter

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

rwols marked 10 inline comments as done. rwols added inline comments. Comment at: clangd/ClangdUnit.cpp:321 + +// Fill in the label, detail, documentation and insertText fields of the +// CompletionItem. ilya-biryukov wrote: > Maybe split writes into

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

rwols added a comment. After digging into VSCode, I now think that presenting snippets is the wrong way forward, and I believe the right way is to implement the `signatureHelpProvider` protocol for method/function parameters. See:

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

rwols created this revision. Enhances CompletionItemsCollector in such a way that snippet completions are presented to the client. This is a work-in-progress. It currently works in Sublime Text 3 using the new "LSP" plugin. In VSCode, the snippets are inserted into the buffer as plaintext without

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

rwols updated this revision to Diff 113650. rwols marked 3 inline comments as done. rwols added a comment. - Split up the CompletionItemsCollector into two classes called PlainTextCompletionItemsCollector and SnippetCompletionItemsCollector to allow collecting both types of items. - Add a

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

rwols marked 4 inline comments as done. rwols added inline comments. Comment at: clangd/ClangdUnit.cpp:419 +assert(item.detail.empty() && "Unexpected extraneous CK_ResultType"); +Item.detail = Chunk.Text; +break; ilya-biryukov wrote: >

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

rwols added a comment. Forgot to mention I've also made sure the `filterText` field is taken care of now (both for snippet and for plaintext items). https://reviews.llvm.org/D37101 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

rwols updated this revision to Diff 113657. rwols added a comment. - Adjust SnippetCompletionItemCollector such that it only sets insertTextFormat to InsertTextFormat::Snippet when it's actually needed (i.e. we encounter a CK_Placeholder chunk). - Fix failing tests in

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

rwols updated this revision to Diff 113258. rwols added a comment. Some more tweaks - Move assert to constructor of CompletionItemsCollector - Use a local variable for the annotations count - Move the documentation handling to its own private const member function

[PATCH] D38048: [clangd] Add textDocument/signatureHelp

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

[PATCH] D38720: [clangd] Report proper kinds for 'Keyword' and 'Snippet' completion items.

rwols accepted this revision. rwols added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D38720 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D38939: [clangd] Handle exit notification (proper shutdown)

rwols updated this revision to Diff 119370. rwols added a comment. - Add test shutdown-with-exit.test - Add test shutdown-without-exit.test - Make protocol.test pass by inverting exit code ("# RUN: not ...") https://reviews.llvm.org/D38939 Files: clangd/ClangdLSPServer.cpp

[PATCH] D38939: [clangd] Handle exit notification (proper shutdown)

rwols added a comment. > So I'm not totally convinced that treating an abrupt exit or EOF as an error > is worth the hassle Agreed, at least from the POV of a client clangd shuts down gracefully now. https://reviews.llvm.org/D38939 ___

[PATCH] D38939: [clangd] Handle exit notification (proper shutdown)

rwols marked 3 inline comments as done. rwols added a comment. > you have commit access now, right? I'll have to think about if I want that responsibility! https://reviews.llvm.org/D38939 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D38939: [clangd] Handle exit notification (proper shutdown)

rwols updated this revision to Diff 119941. rwols added a comment. - Clarify why we invert the tests in protocol.test, - Use a variable name to clarify why we exit with status code 1 (when run() returns true), - Reword misleading comment in onShutdown method. https://reviews.llvm.org/D38939

[PATCH] D39430: [clangd] formatting: don't ignore style

rwols updated this revision to Diff 122904. rwols marked an inline comment as done. rwols added a comment. Removed braces around single statement if-else. https://reviews.llvm.org/D39430 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/ClangdServer.h

[PATCH] D39430: [clangd] formatting: don't ignore style

rwols marked 2 inline comments as done. rwols added inline comments. Comment at: clangd/ClangdServer.h:289 + llvm::Expected + formatRange(llvm::StringRef Code, PathRef File, Range Rng); + ilya-biryukov wrote: > Why do we accept `Code` as a

[PATCH] D39430: [clangd] formatting: don't ignore style

rwols added inline comments. Comment at: clangd/ClangdUnit.cpp:168 - P.line = Location.getSpellingLineNumber() - 1; - P.character = Location.getSpellingColumnNumber(); Range R = {P, P}; Why subtract 1 from the line here, but not from the column? When

[PATCH] D39430: [clangd] formatting: don't ignore style

rwols updated this revision to Diff 124307. rwols added a comment. - Rebase to latest upstream revision. - Go all-in with TextEdit, even down to ClangdUnit.cpp. - Move FixItsMap to ClangdServer. ClangdLSPServer is much cleaner now. - Remove the cached FixIts when the client closes the document. -

[PATCH] D40860: [clangd] Fix diagnostic positions

rwols closed this revision. rwols added a comment. https://reviews.llvm.org/D41118 fixes both the start positions as well as the ranges, let's merge that. https://reviews.llvm.org/D40860 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D41118: [clangd] Emit ranges for clangd diagnostics, and fix off-by-one positions

rwols accepted this revision. rwols added a comment. This revision is now accepted and ready to land. LGTM, you can go ahead and land this, I'll drop https://reviews.llvm.org/D40860. I'm happy we got to the bottom of this :) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41118

[PATCH] D41031: [clangd] (Attempt to) read clang-format file for document formatting

This revision was automatically updated to reflect the committed changes. Closed by commit rL320524: [clangd] (Attempt to) read clang-format file for document formatting (authored by rwols, committed by ). Herald added a subscriber: klimek. Changed prior to commit:

[PATCH] D39430: [clangd] formatting: don't ignore style

rwols added inline comments. Comment at: clangd/ClangdServer.h:289 + llvm::Expected + formatRange(llvm::StringRef Code, PathRef File, Range Rng); + ilya-biryukov wrote: > rwols wrote: > > ilya-biryukov wrote: > > > Why do we accept `Code` as a

[PATCH] D39430: [clangd] formatting: don't ignore style

rwols added inline comments. Comment at: clangd/ClangdServer.h:289 + llvm::Expected + formatRange(llvm::StringRef Code, PathRef File, Range Rng); + rwols wrote: > ilya-biryukov wrote: > > rwols wrote: > > > ilya-biryukov wrote: > > > > Why do we

[PATCH] D39430: [clangd] formatting: don't ignore style

rwols updated this revision to Diff 122606. rwols added a comment. - Merge with upstream, fix merge conflicts - Add a FIXME for a caching strategy for .clang-format files https://reviews.llvm.org/D39430 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/ClangdServer.h

[PATCH] D39430: [clangd] formatting: don't ignore style

rwols marked 4 inline comments as done. rwols added a comment. Is this ready to merge? I'd like to implement tests in another differential, I'm having trouble referencing a temp dir from `lit` in a JSON request to clangd. Comment at: clangd/ClangdServer.cpp:429 + auto

[PATCH] D39430: [clangd] formatting: don't ignore style

rwols updated this revision to Diff 121524. rwols added a comment. Use llvm::Error for beautiful handling of errors. It's perhaps an idea to write a `replyExpected(llvm::Expected)`, that will transparently handle `llvm::Error`s. I'll write a unit test for these changes next. It's probably best

[PATCH] D39430: [clangd] formatting: don't ignore style

rwols marked 2 inline comments as done. rwols added inline comments. Comment at: clangd/ClangdServer.cpp:41 // Call clang-format. - // FIXME: Don't ignore style. - format::FormatStyle Style = format::getLLVMStyle(); - auto Result = format::reformat(Style, Code, Ranges,

[PATCH] D39430: [clangd] formatting: don't ignore style

rwols updated this revision to Diff 121605. rwols added a comment. - Rebase to latest SVN revision. - Use `void reply(llvm:Expected Result)` for transparent error handling. - Use `FSProvider` from `ClangdServer` to provide a filesystem for `format::getStyle`. - Organize things around

[PATCH] D39430: [clangd] various fixes

rwols added a comment. I will split this diff up into separate parts. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D39430 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D41031: [clangd] (Attempt to) read clang-format file for document formatting

rwols created this revision. Takes into account the clang-format file of the project, if any. Reverts to LLVM if nothing is found. Replies with an error if any error occured. For instance, a parse error in the clang-format YAML file. https://reviews.llvm.org/D41031 Files:

[PATCH] D39430: [clangd] formatting: don't ignore style

rwols updated this revision to Diff 125267. rwols added a comment. Herald added a subscriber: klimek. - Merge with upstream revision 319613 - Fix FixItHints not getting applied correctly in all cases. The problem is that clang::CharSourceRange can either be a TokenRange or a CharRange. In the

[PATCH] D40860: [clangd] Fix diagnostic positions

rwols created this revision. This translates diagnostics correctly between 0-based LSP columns, and 1-based clang columns. https://reviews.llvm.org/D40860 Files: clang-tools-extra/clangd/ClangdLSPServer.cpp clang-tools-extra/clangd/ClangdLSPServer.h

[PATCH] D39430: [clangd] various fixes

rwols added a comment. This differential is now split into https://reviews.llvm.org/D41031 and https://reviews.llvm.org/D40860. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D39430 ___ cfe-commits mailing list

[PATCH] D39430: [clangd] formatting: don't ignore style

rwols added a comment. Does anyone have an idea how I would write a test for this? Also, I should have commit access after approval now. So that's less work for you guys ;) https://reviews.llvm.org/D39430 ___ cfe-commits mailing list

[PATCH] D39430: [clangd] formatting: don't ignore style

rwols created this revision. Look up in parent directories for a .clang-format file. Use "LLVM" as fallback style. https://reviews.llvm.org/D39430 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/ClangdServer.cpp clangd/ClangdServer.h Index: clangd/ClangdServer.h

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

rwols added a comment. Can we get this merged soon? I'm experiencing a lot of false positive diagnostics with this library at work. Repository: rC Clang https://reviews.llvm.org/D44480 ___ cfe-commits

[PATCH] D49967: [clangd] Add command-line option to suppress the space and the circular dot prepended in a completion label.

rwols created this revision. rwols added reviewers: sammccall, ilya-biryukov, ioeric. Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay. Some LSP clients (e.g. Sublime Text) assume that the label and the completion trigger have a common prefix. This assumption is broken by

[PATCH] D49967: [clangd] Add command-line option to suppress the space and the circular dot prepended in a completion label.

rwols closed this revision. rwols added a comment. Oh... I should have used `arc land` instead of `svn commit`. Here's the commit: https://reviews.llvm.org/rCTE338223 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49967 ___

[PATCH] D49967: [clangd] Add command-line option to suppress the space and the circular dot prepended in a completion label.

rwols updated this revision to Diff 157894. rwols added a comment. Avoid double negative for command line option Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49967 Files: clangd/tool/ClangdMain.cpp Index: clangd/tool/ClangdMain.cpp