Re: [PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[David Blaikie via cfe-commits]

I mention it only out of interest of deduplication of functionality/code
within the LLVM project as a whole. Be nice not to maintain two things if
one would suffice.

On Thu, Oct 12, 2017 at 6:21 AM Sam McCall  wrote:

> Interesting - this is pretty primitive, and still fairly tightly coupled
> to JSON-RPC.
> I can't easily tell from the code how the ORC RPC functionality - would it
> be easy to use with JSON-RPC, does it make sense to use serialization only,
> does it have opinions about threading models? And really, what would we
> expect to gain vs using the current more naive code (it looks more
> complicated, probably having more complex requirements).
>
> On Mon, Oct 9, 2017 at 11:16 PM, David Blaikie  wrote:
>
>> hey Lang (& folks here) any chance there's some overlap between the RPC
>> functionality here and the RPC functionality in ORC that could be
>> deduplicated/refactored?
>>
>> On Fri, Oct 6, 2017 at 5:30 AM Ilya Biryukov via Phabricator via
>> cfe-commits  wrote:
>>
>>> ilya-biryukov accepted this revision.
>>> ilya-biryukov added a comment.
>>> This revision is now accepted and ready to land.
>>>
>>> LGTM.
>>> Note there's a new LSP method handler added upstream
>>> (`textDocument/signatureHelp`), we should add it to this change before
>>> submitting.
>>>
>>>
>>>
>>> 
>>> Comment at: clangd/ClangdLSPServer.h:47
>>>// Implement ProtocolCallbacks.
>>> -  void onInitialize(StringRef ID, InitializeParams IP,
>>> -JSONOutput ) override;
>>> -  void onShutdown(JSONOutput ) override;
>>> -  void onDocumentDidOpen(DidOpenTextDocumentParams Params,
>>> - JSONOutput ) override;
>>> -  void onDocumentDidChange(DidChangeTextDocumentParams Params,
>>> -   JSONOutput ) override;
>>> -  void onDocumentDidClose(DidCloseTextDocumentParams Params,
>>> -  JSONOutput ) override;
>>> -  void onDocumentOnTypeFormatting(DocumentOnTypeFormattingParams Params,
>>> -  StringRef ID, JSONOutput )
>>> override;
>>> -  void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
>>> - StringRef ID, JSONOutput )
>>> override;
>>> -  void onDocumentFormatting(DocumentFormattingParams Params, StringRef
>>> ID,
>>> -JSONOutput ) override;
>>> -  void onCodeAction(CodeActionParams Params, StringRef ID,
>>> -JSONOutput ) override;
>>> -  void onCompletion(TextDocumentPositionParams Params, StringRef ID,
>>> -JSONOutput ) override;
>>> -  void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
>>> -JSONOutput ) override;
>>> -  void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
>>> -JSONOutput ) override;
>>> +  void onInitialize(Ctx , InitializeParams ) override;
>>> +  void onShutdown(Ctx , NoParams ) override;
>>> 
>>> sammccall wrote:
>>> > ilya-biryukov wrote:
>>> > > ilya-biryukov wrote:
>>> > > > Maybe there's a way to have a typed return value instead of `Ctx`?
>>> > > > This would give an interface that's harder to misuse: one can't
>>> forget to call `reply` or call it twice.
>>> > > >
>>> > > > Here's on design that comes to mind.
>>> > > > Notification handlers could have `void` return, normal requests
>>> can return `Optional` or `Optional` (with result json).
>>> > > > `Optional` is be used to indicate whether error occurred while
>>> processing the request.
>>> > > >
>>> > > After putting more thought into it, `Ctx`-based API seems to have an
>>> advantage: it will allow us to easily implement async responses.
>>> > > E.g., we can run code completion on a background thread and continue
>>> processing other requests. When completion is ready, we will simply call
>>> `Ctx.reply` to return results to the language client.
>>> > >
>>> > > To make that possible, can we allow moving `RequestContext` and pass
>>> it by-value instead of by-ref?
>>> > Yeah I thought about returning a value... it certainly reads more
>>> nicely, but I don't think we're ready to do a good job in this patch:
>>> >  - return value should be an object ready to be serialized (rather
>>> than a JSON string) - I don't want to bring that in scope here, but it
>>> might affect the details of the API
>>> >  - there's several cases we know about (return object, no reply, error
>>> reply) and some we're not sure about (async as you mention - any multiple
>>> responses)? I think this needs some design, and I don't yet know the
>>> project well enough to drive it.
>>> >
>>> > I've switched to passing Ctx by value as you suggest (though it's
>>> certainly cheap enough to copy, too).
>>> Yeah, copy is also fine there performance-wise.
>>>
>>> I do think move-only interface fits slightly better. We can check a
>>> whole bunch of 

[PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL315577: [clangd] less boilerplate in RPC dispatch (authored 
by sammccall).

Repository:
  rL LLVM

https://reviews.llvm.org/D38464

Files:
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/ClangdLSPServer.h
  clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
  clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h
  clang-tools-extra/trunk/clangd/Protocol.cpp
  clang-tools-extra/trunk/clangd/Protocol.h
  clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
  clang-tools-extra/trunk/clangd/ProtocolHandlers.h
  clang-tools-extra/trunk/test/clangd/did-change-watch-files.test
  clang-tools-extra/trunk/test/clangd/fixits.test

Index: clang-tools-extra/trunk/test/clangd/fixits.test
===
--- clang-tools-extra/trunk/test/clangd/fixits.test
+++ clang-tools-extra/trunk/test/clangd/fixits.test
@@ -15,13 +15,13 @@
 
  {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 #
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
 #
 Content-Length: 771
 
 {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"code":"1","source":"foo","message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 # Make sure unused "code" and "source" fields ignored gracefully
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, 

Re: [PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[Sam McCall via cfe-commits]

Interesting - this is pretty primitive, and still fairly tightly coupled to
JSON-RPC.
I can't easily tell from the code how the ORC RPC functionality - would it
be easy to use with JSON-RPC, does it make sense to use serialization only,
does it have opinions about threading models? And really, what would we
expect to gain vs using the current more naive code (it looks more
complicated, probably having more complex requirements).

On Mon, Oct 9, 2017 at 11:16 PM, David Blaikie  wrote:

> hey Lang (& folks here) any chance there's some overlap between the RPC
> functionality here and the RPC functionality in ORC that could be
> deduplicated/refactored?
>
> On Fri, Oct 6, 2017 at 5:30 AM Ilya Biryukov via Phabricator via
> cfe-commits  wrote:
>
>> ilya-biryukov accepted this revision.
>> ilya-biryukov added a comment.
>> This revision is now accepted and ready to land.
>>
>> LGTM.
>> Note there's a new LSP method handler added upstream
>> (`textDocument/signatureHelp`), we should add it to this change before
>> submitting.
>>
>>
>>
>> 
>> Comment at: clangd/ClangdLSPServer.h:47
>>// Implement ProtocolCallbacks.
>> -  void onInitialize(StringRef ID, InitializeParams IP,
>> -JSONOutput ) override;
>> -  void onShutdown(JSONOutput ) override;
>> -  void onDocumentDidOpen(DidOpenTextDocumentParams Params,
>> - JSONOutput ) override;
>> -  void onDocumentDidChange(DidChangeTextDocumentParams Params,
>> -   JSONOutput ) override;
>> -  void onDocumentDidClose(DidCloseTextDocumentParams Params,
>> -  JSONOutput ) override;
>> -  void onDocumentOnTypeFormatting(DocumentOnTypeFormattingParams Params,
>> -  StringRef ID, JSONOutput )
>> override;
>> -  void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
>> - StringRef ID, JSONOutput ) override;
>> -  void onDocumentFormatting(DocumentFormattingParams Params, StringRef
>> ID,
>> -JSONOutput ) override;
>> -  void onCodeAction(CodeActionParams Params, StringRef ID,
>> -JSONOutput ) override;
>> -  void onCompletion(TextDocumentPositionParams Params, StringRef ID,
>> -JSONOutput ) override;
>> -  void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
>> -JSONOutput ) override;
>> -  void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
>> -JSONOutput ) override;
>> +  void onInitialize(Ctx , InitializeParams ) override;
>> +  void onShutdown(Ctx , NoParams ) override;
>> 
>> sammccall wrote:
>> > ilya-biryukov wrote:
>> > > ilya-biryukov wrote:
>> > > > Maybe there's a way to have a typed return value instead of `Ctx`?
>> > > > This would give an interface that's harder to misuse: one can't
>> forget to call `reply` or call it twice.
>> > > >
>> > > > Here's on design that comes to mind.
>> > > > Notification handlers could have `void` return, normal requests can
>> return `Optional` or `Optional` (with result json).
>> > > > `Optional` is be used to indicate whether error occurred while
>> processing the request.
>> > > >
>> > > After putting more thought into it, `Ctx`-based API seems to have an
>> advantage: it will allow us to easily implement async responses.
>> > > E.g., we can run code completion on a background thread and continue
>> processing other requests. When completion is ready, we will simply call
>> `Ctx.reply` to return results to the language client.
>> > >
>> > > To make that possible, can we allow moving `RequestContext` and pass
>> it by-value instead of by-ref?
>> > Yeah I thought about returning a value... it certainly reads more
>> nicely, but I don't think we're ready to do a good job in this patch:
>> >  - return value should be an object ready to be serialized (rather than
>> a JSON string) - I don't want to bring that in scope here, but it might
>> affect the details of the API
>> >  - there's several cases we know about (return object, no reply, error
>> reply) and some we're not sure about (async as you mention - any multiple
>> responses)? I think this needs some design, and I don't yet know the
>> project well enough to drive it.
>> >
>> > I've switched to passing Ctx by value as you suggest (though it's
>> certainly cheap enough to copy, too).
>> Yeah, copy is also fine there performance-wise.
>>
>> I do think move-only interface fits slightly better. We can check a whole
>> bunch of invariants if `Ctx` is move-only (i.e., that request wasn't
>> dropped without response or `reply` was not called twice).
>>
>>
>> 
>> Comment at: clangd/ClangdLSPServer.h:48
>> +  void onInitialize(Ctx , InitializeParams ) override;
>> +  void onShutdown(Ctx , NoParams ) override;
>> +  void onDocumentDidOpen(Ctx , 

[PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 118782.
sammccall added a comment.

Rebase


https://reviews.llvm.org/D38464

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/JSONRPCDispatcher.cpp
  clangd/JSONRPCDispatcher.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/did-change-watch-files.test
  test/clangd/fixits.test

Index: test/clangd/fixits.test
===
--- test/clangd/fixits.test
+++ test/clangd/fixits.test
@@ -15,13 +15,13 @@
 
  {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 #
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
 #
 Content-Length: 771
 
 {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"code":"1","source":"foo","message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 # Make sure unused "code" and "source" fields ignored gracefully
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
 #
 Content-Length: 44
 
Index: test/clangd/did-change-watch-files.test

Re: [PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[David Blaikie via cfe-commits]

hey Lang (& folks here) any chance there's some overlap between the RPC
functionality here and the RPC functionality in ORC that could be
deduplicated/refactored?

On Fri, Oct 6, 2017 at 5:30 AM Ilya Biryukov via Phabricator via
cfe-commits  wrote:

> ilya-biryukov accepted this revision.
> ilya-biryukov added a comment.
> This revision is now accepted and ready to land.
>
> LGTM.
> Note there's a new LSP method handler added upstream
> (`textDocument/signatureHelp`), we should add it to this change before
> submitting.
>
>
>
> 
> Comment at: clangd/ClangdLSPServer.h:47
>// Implement ProtocolCallbacks.
> -  void onInitialize(StringRef ID, InitializeParams IP,
> -JSONOutput ) override;
> -  void onShutdown(JSONOutput ) override;
> -  void onDocumentDidOpen(DidOpenTextDocumentParams Params,
> - JSONOutput ) override;
> -  void onDocumentDidChange(DidChangeTextDocumentParams Params,
> -   JSONOutput ) override;
> -  void onDocumentDidClose(DidCloseTextDocumentParams Params,
> -  JSONOutput ) override;
> -  void onDocumentOnTypeFormatting(DocumentOnTypeFormattingParams Params,
> -  StringRef ID, JSONOutput ) override;
> -  void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
> - StringRef ID, JSONOutput ) override;
> -  void onDocumentFormatting(DocumentFormattingParams Params, StringRef ID,
> -JSONOutput ) override;
> -  void onCodeAction(CodeActionParams Params, StringRef ID,
> -JSONOutput ) override;
> -  void onCompletion(TextDocumentPositionParams Params, StringRef ID,
> -JSONOutput ) override;
> -  void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
> -JSONOutput ) override;
> -  void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
> -JSONOutput ) override;
> +  void onInitialize(Ctx , InitializeParams ) override;
> +  void onShutdown(Ctx , NoParams ) override;
> 
> sammccall wrote:
> > ilya-biryukov wrote:
> > > ilya-biryukov wrote:
> > > > Maybe there's a way to have a typed return value instead of `Ctx`?
> > > > This would give an interface that's harder to misuse: one can't
> forget to call `reply` or call it twice.
> > > >
> > > > Here's on design that comes to mind.
> > > > Notification handlers could have `void` return, normal requests can
> return `Optional` or `Optional` (with result json).
> > > > `Optional` is be used to indicate whether error occurred while
> processing the request.
> > > >
> > > After putting more thought into it, `Ctx`-based API seems to have an
> advantage: it will allow us to easily implement async responses.
> > > E.g., we can run code completion on a background thread and continue
> processing other requests. When completion is ready, we will simply call
> `Ctx.reply` to return results to the language client.
> > >
> > > To make that possible, can we allow moving `RequestContext` and pass
> it by-value instead of by-ref?
> > Yeah I thought about returning a value... it certainly reads more
> nicely, but I don't think we're ready to do a good job in this patch:
> >  - return value should be an object ready to be serialized (rather than
> a JSON string) - I don't want to bring that in scope here, but it might
> affect the details of the API
> >  - there's several cases we know about (return object, no reply, error
> reply) and some we're not sure about (async as you mention - any multiple
> responses)? I think this needs some design, and I don't yet know the
> project well enough to drive it.
> >
> > I've switched to passing Ctx by value as you suggest (though it's
> certainly cheap enough to copy, too).
> Yeah, copy is also fine there performance-wise.
>
> I do think move-only interface fits slightly better. We can check a whole
> bunch of invariants if `Ctx` is move-only (i.e., that request wasn't
> dropped without response or `reply` was not called twice).
>
>
> 
> Comment at: clangd/ClangdLSPServer.h:48
> +  void onInitialize(Ctx , InitializeParams ) override;
> +  void onShutdown(Ctx , NoParams ) override;
> +  void onDocumentDidOpen(Ctx , DidOpenTextDocumentParams )
> override;
> 
> sammccall wrote:
> > ilya-biryukov wrote:
> > > Maybe there's a way to get rid of `NoParams`?
> > > E.g. by adding a overload to `HandlerRegisterer`?
> > Even if there was, I think I prefer the regularity (changed this to
> ShutdownParams - oops!).
> >
> > Otherwise the signature's dependent on some combination of {current LSP,
> whether we actually implement the options, whether we've defined any
> extensions}. It's harder to remember, means changing lots of places when
> these factors change, and complicates the generic code.
> >
> > Maybe I've just spent too long 

[PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM.
Note there's a new LSP method handler added upstream 
(`textDocument/signatureHelp`), we should add it to this change before 
submitting.




Comment at: clangd/ClangdLSPServer.h:47
   // Implement ProtocolCallbacks.
-  void onInitialize(StringRef ID, InitializeParams IP,
-JSONOutput ) override;
-  void onShutdown(JSONOutput ) override;
-  void onDocumentDidOpen(DidOpenTextDocumentParams Params,
- JSONOutput ) override;
-  void onDocumentDidChange(DidChangeTextDocumentParams Params,
-   JSONOutput ) override;
-  void onDocumentDidClose(DidCloseTextDocumentParams Params,
-  JSONOutput ) override;
-  void onDocumentOnTypeFormatting(DocumentOnTypeFormattingParams Params,
-  StringRef ID, JSONOutput ) override;
-  void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
- StringRef ID, JSONOutput ) override;
-  void onDocumentFormatting(DocumentFormattingParams Params, StringRef ID,
-JSONOutput ) override;
-  void onCodeAction(CodeActionParams Params, StringRef ID,
-JSONOutput ) override;
-  void onCompletion(TextDocumentPositionParams Params, StringRef ID,
-JSONOutput ) override;
-  void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
-JSONOutput ) override;
-  void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
-JSONOutput ) override;
+  void onInitialize(Ctx , InitializeParams ) override;
+  void onShutdown(Ctx , NoParams ) override;

sammccall wrote:
> ilya-biryukov wrote:
> > ilya-biryukov wrote:
> > > Maybe there's a way to have a typed return value instead of `Ctx`?
> > > This would give an interface that's harder to misuse: one can't forget to 
> > > call `reply` or call it twice.
> > > 
> > > Here's on design that comes to mind.
> > > Notification handlers could have `void` return, normal requests can 
> > > return `Optional` or `Optional` (with result json).
> > > `Optional` is be used to indicate whether error occurred while processing 
> > > the request.
> > > 
> > After putting more thought into it, `Ctx`-based API seems to have an 
> > advantage: it will allow us to easily implement async responses.
> > E.g., we can run code completion on a background thread and continue 
> > processing other requests. When completion is ready, we will simply call 
> > `Ctx.reply` to return results to the language client.
> > 
> > To make that possible, can we allow moving `RequestContext` and pass it 
> > by-value instead of by-ref?
> Yeah I thought about returning a value... it certainly reads more nicely, but 
> I don't think we're ready to do a good job in this patch:
>  - return value should be an object ready to be serialized (rather than a 
> JSON string) - I don't want to bring that in scope here, but it might affect 
> the details of the API
>  - there's several cases we know about (return object, no reply, error reply) 
> and some we're not sure about (async as you mention - any multiple 
> responses)? I think this needs some design, and I don't yet know the project 
> well enough to drive it.
> 
> I've switched to passing Ctx by value as you suggest (though it's certainly 
> cheap enough to copy, too).
Yeah, copy is also fine there performance-wise. 

I do think move-only interface fits slightly better. We can check a whole bunch 
of invariants if `Ctx` is move-only (i.e., that request wasn't dropped without 
response or `reply` was not called twice).



Comment at: clangd/ClangdLSPServer.h:48
+  void onInitialize(Ctx , InitializeParams ) override;
+  void onShutdown(Ctx , NoParams ) override;
+  void onDocumentDidOpen(Ctx , DidOpenTextDocumentParams ) override;

sammccall wrote:
> ilya-biryukov wrote:
> > Maybe there's a way to get rid of `NoParams`?
> > E.g. by adding a overload to `HandlerRegisterer`?
> Even if there was, I think I prefer the regularity (changed this to 
> ShutdownParams - oops!).
> 
> Otherwise the signature's dependent on some combination of {current LSP, 
> whether we actually implement the options, whether we've defined any 
> extensions}. It's harder to remember, means changing lots of places when 
> these factors change, and complicates the generic code.
> 
> Maybe I've just spent too long around Stubby, though - WDYT?
All those are good points. We also have only one method that does not have 
params currently, so it's probably not even worth additional complexity in the 
implementation.


https://reviews.llvm.org/D38464



___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks! All addressed (except removing ShutdownParams, which I'm not sure is 
worthwhile).




Comment at: clangd/ClangdLSPServer.h:47
   // Implement ProtocolCallbacks.
-  void onInitialize(StringRef ID, InitializeParams IP,
-JSONOutput ) override;
-  void onShutdown(JSONOutput ) override;
-  void onDocumentDidOpen(DidOpenTextDocumentParams Params,
- JSONOutput ) override;
-  void onDocumentDidChange(DidChangeTextDocumentParams Params,
-   JSONOutput ) override;
-  void onDocumentDidClose(DidCloseTextDocumentParams Params,
-  JSONOutput ) override;
-  void onDocumentOnTypeFormatting(DocumentOnTypeFormattingParams Params,
-  StringRef ID, JSONOutput ) override;
-  void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
- StringRef ID, JSONOutput ) override;
-  void onDocumentFormatting(DocumentFormattingParams Params, StringRef ID,
-JSONOutput ) override;
-  void onCodeAction(CodeActionParams Params, StringRef ID,
-JSONOutput ) override;
-  void onCompletion(TextDocumentPositionParams Params, StringRef ID,
-JSONOutput ) override;
-  void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
-JSONOutput ) override;
-  void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
-JSONOutput ) override;
+  void onInitialize(Ctx , InitializeParams ) override;
+  void onShutdown(Ctx , NoParams ) override;

ilya-biryukov wrote:
> ilya-biryukov wrote:
> > Maybe there's a way to have a typed return value instead of `Ctx`?
> > This would give an interface that's harder to misuse: one can't forget to 
> > call `reply` or call it twice.
> > 
> > Here's on design that comes to mind.
> > Notification handlers could have `void` return, normal requests can return 
> > `Optional` or `Optional` (with result json).
> > `Optional` is be used to indicate whether error occurred while processing 
> > the request.
> > 
> After putting more thought into it, `Ctx`-based API seems to have an 
> advantage: it will allow us to easily implement async responses.
> E.g., we can run code completion on a background thread and continue 
> processing other requests. When completion is ready, we will simply call 
> `Ctx.reply` to return results to the language client.
> 
> To make that possible, can we allow moving `RequestContext` and pass it 
> by-value instead of by-ref?
Yeah I thought about returning a value... it certainly reads more nicely, but I 
don't think we're ready to do a good job in this patch:
 - return value should be an object ready to be serialized (rather than a JSON 
string) - I don't want to bring that in scope here, but it might affect the 
details of the API
 - there's several cases we know about (return object, no reply, error reply) 
and some we're not sure about (async as you mention - any multiple responses)? 
I think this needs some design, and I don't yet know the project well enough to 
drive it.

I've switched to passing Ctx by value as you suggest (though it's certainly 
cheap enough to copy, too).



Comment at: clangd/ClangdLSPServer.h:48
+  void onInitialize(Ctx , InitializeParams ) override;
+  void onShutdown(Ctx , NoParams ) override;
+  void onDocumentDidOpen(Ctx , DidOpenTextDocumentParams ) override;

ilya-biryukov wrote:
> Maybe there's a way to get rid of `NoParams`?
> E.g. by adding a overload to `HandlerRegisterer`?
Even if there was, I think I prefer the regularity (changed this to 
ShutdownParams - oops!).

Otherwise the signature's dependent on some combination of {current LSP, 
whether we actually implement the options, whether we've defined any 
extensions}. It's harder to remember, means changing lots of places when these 
factors change, and complicates the generic code.

Maybe I've just spent too long around Stubby, though - WDYT?


https://reviews.llvm.org/D38464



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 117835.
sammccall marked 5 inline comments as done.
sammccall added a comment.

- ClangLSPServer.h should refer to ShutdownParams, not NoParams
- Address review comments


https://reviews.llvm.org/D38464

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/JSONRPCDispatcher.cpp
  clangd/JSONRPCDispatcher.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/fixits.test

Index: test/clangd/fixits.test
===
--- test/clangd/fixits.test
+++ test/clangd/fixits.test
@@ -15,13 +15,13 @@
 
  {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 #
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
 #
 Content-Length: 771
 
 {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"code":"1","source":"foo","message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 # Make sure unused "code" and "source" fields ignored gracefully
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
 #
 Content-Length: 44
 
Index: 

[PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdLSPServer.h:47
   // Implement ProtocolCallbacks.
-  void onInitialize(StringRef ID, InitializeParams IP,
-JSONOutput ) override;
-  void onShutdown(JSONOutput ) override;
-  void onDocumentDidOpen(DidOpenTextDocumentParams Params,
- JSONOutput ) override;
-  void onDocumentDidChange(DidChangeTextDocumentParams Params,
-   JSONOutput ) override;
-  void onDocumentDidClose(DidCloseTextDocumentParams Params,
-  JSONOutput ) override;
-  void onDocumentOnTypeFormatting(DocumentOnTypeFormattingParams Params,
-  StringRef ID, JSONOutput ) override;
-  void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
- StringRef ID, JSONOutput ) override;
-  void onDocumentFormatting(DocumentFormattingParams Params, StringRef ID,
-JSONOutput ) override;
-  void onCodeAction(CodeActionParams Params, StringRef ID,
-JSONOutput ) override;
-  void onCompletion(TextDocumentPositionParams Params, StringRef ID,
-JSONOutput ) override;
-  void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
-JSONOutput ) override;
-  void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
-JSONOutput ) override;
+  void onInitialize(Ctx , InitializeParams ) override;
+  void onShutdown(Ctx , NoParams ) override;

ilya-biryukov wrote:
> Maybe there's a way to have a typed return value instead of `Ctx`?
> This would give an interface that's harder to misuse: one can't forget to 
> call `reply` or call it twice.
> 
> Here's on design that comes to mind.
> Notification handlers could have `void` return, normal requests can return 
> `Optional` or `Optional` (with result json).
> `Optional` is be used to indicate whether error occurred while processing the 
> request.
> 
After putting more thought into it, `Ctx`-based API seems to have an advantage: 
it will allow us to easily implement async responses.
E.g., we can run code completion on a background thread and continue processing 
other requests. When completion is ready, we will simply call `Ctx.reply` to 
return results to the language client.

To make that possible, can we allow moving `RequestContext` and pass it 
by-value instead of by-ref?



Comment at: clangd/JSONRPCDispatcher.h:55
+  /// Logs a message locally only. A newline will be added.
+  void log(const Twine ) { Out.log(Message + "\n"); }
+

It seems there are no any of `log`. Maybe remove it from this class?


https://reviews.llvm.org/D38464



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdLSPServer.h:47
   // Implement ProtocolCallbacks.
-  void onInitialize(StringRef ID, InitializeParams IP,
-JSONOutput ) override;
-  void onShutdown(JSONOutput ) override;
-  void onDocumentDidOpen(DidOpenTextDocumentParams Params,
- JSONOutput ) override;
-  void onDocumentDidChange(DidChangeTextDocumentParams Params,
-   JSONOutput ) override;
-  void onDocumentDidClose(DidCloseTextDocumentParams Params,
-  JSONOutput ) override;
-  void onDocumentOnTypeFormatting(DocumentOnTypeFormattingParams Params,
-  StringRef ID, JSONOutput ) override;
-  void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
- StringRef ID, JSONOutput ) override;
-  void onDocumentFormatting(DocumentFormattingParams Params, StringRef ID,
-JSONOutput ) override;
-  void onCodeAction(CodeActionParams Params, StringRef ID,
-JSONOutput ) override;
-  void onCompletion(TextDocumentPositionParams Params, StringRef ID,
-JSONOutput ) override;
-  void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
-JSONOutput ) override;
-  void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
-JSONOutput ) override;
+  void onInitialize(Ctx , InitializeParams ) override;
+  void onShutdown(Ctx , NoParams ) override;

Maybe there's a way to have a typed return value instead of `Ctx`?
This would give an interface that's harder to misuse: one can't forget to call 
`reply` or call it twice.

Here's on design that comes to mind.
Notification handlers could have `void` return, normal requests can return 
`Optional` or `Optional` (with result json).
`Optional` is be used to indicate whether error occurred while processing the 
request.




Comment at: clangd/ClangdLSPServer.h:48
+  void onInitialize(Ctx , InitializeParams ) override;
+  void onShutdown(Ctx , NoParams ) override;
+  void onDocumentDidOpen(Ctx , DidOpenTextDocumentParams ) override;

Maybe there's a way to get rid of `NoParams`?
E.g. by adding a overload to `HandlerRegisterer`?



Comment at: clangd/Protocol.cpp:317
+  return ParseFailure();
+;
 

Why do we need these empty `;` statements?



Comment at: clangd/ProtocolHandlers.cpp:14
 #include "DraftStore.h"
-using namespace clang;
-using namespace clangd;

Maybe move this into a separate commit that would also replace `using 
namespace` in all other files?



Comment at: clangd/ProtocolHandlers.cpp:25
+  template 
+  void operator()(StringRef method,
+  void (ProtocolCallbacks::*Handler)(RequestContext &, Param)) 
{

Code style: parameter name should be `Method`


https://reviews.llvm.org/D38464



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 117374.
sammccall added a comment.

- clang-format


https://reviews.llvm.org/D38464

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/JSONRPCDispatcher.cpp
  clangd/JSONRPCDispatcher.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/fixits.test

Index: test/clangd/fixits.test
===
--- test/clangd/fixits.test
+++ test/clangd/fixits.test
@@ -15,13 +15,13 @@
 
  {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 #
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
 #
 Content-Length: 771
 
 {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"code":"1","source":"foo","message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 # Make sure unused "code" and "source" fields ignored gracefully
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
 #
 Content-Length: 44
 
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ 

[PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.

Make the ProtocolHandlers glue between JSONRPCDispatcher and
ClangdLSPServer generic.
Eliminate small differences between methods, de-emphasize the unimportant
distinction between notifications and methods.

ClangdLSPServer is no longer responsible for producing a complete
JSON-RPC response, just the JSON of the result object. (In future, we
should move that JSON serialization out, too).
Handler methods now take a context object that we may hang more
functionality off in the future.

Added documentation to ProtocolHandlers.


https://reviews.llvm.org/D38464

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/JSONRPCDispatcher.cpp
  clangd/JSONRPCDispatcher.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/fixits.test

Index: test/clangd/fixits.test
===
--- test/clangd/fixits.test
+++ test/clangd/fixits.test
@@ -15,13 +15,13 @@
 
  {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 #
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
 #
 Content-Length: 771
 
 {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"code":"1","source":"foo","message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 # Make sure unused "code" and "source" fields ignored gracefully
-# CHECK: {"jsonrpc":"2.0","id":2, "result": [{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end":