sammccall added inline comments.
================
Comment at: clangd/ClangdLSPServer.cpp:338
+ Command Cmd;
+ if (Action.command && Action.edit)
+ return llvm::None;
----------------
kadircet wrote:
> What would you think about emitting two commands in this case? First the edit
> and then the command. I believe LSP doesn't specify any ordering on how the
> commands returned should be executed by the client, so I am OK with current
> state as well. Just wanted to know if there were any other concerns.
That doesn't have the right semantics. Multiple commands returned from
`textDocument/codeAction` are alternatives that the user should select between,
whereas having both an edit and a command means they should be performed
atomically as one action.
(This never actually happens as we don't emit such actions, added a comment)
================
Comment at: clangd/ClangdLSPServer.cpp:350
+ if (Action.kind && *Action.kind == CodeAction::QUICKFIX_KIND)
+ Cmd.title = "Apply fix: " + Cmd.title;
+ return Cmd;
----------------
kadircet wrote:
> It seems we only prepend title with Apply fix when we fallback, I believe it
> would be better to add them in CodeAction instead?
The `CodeAction` has a slot to describe the type of action: if the client wants
to prepend "Quick Fix: " or so it can.
(Personally this just seems like noise to me, so I'd rather the client omit it,
but...)
================
Comment at: clangd/ClangdLSPServer.cpp:355
void ClangdLSPServer::onCodeAction(CodeActionParams &Params) {
// We provide a code action for each diagnostic at the requested location
// which has FixIts available.
----------------
kadircet wrote:
> I believe this comment is misleading, do we perform any location check? Maybe
> change that to say "requested file"?
Updated the comment.
================
Comment at: clangd/Protocol.h:390
+ /// Flattened from codeAction.codeActionLiteralSupport.
+ // FIXME: flatten other properties in this way.
+ bool codeActionLiteralSupport = false;
----------------
kadircet wrote:
> What is the reason behind this one? Is it because clients must handle unknown
> items on their own and fallback to a default one?
>
> Since that default is client specific, behavior might change from client to
> client. I agree that clients should be up-to-date with the specs and handle
> all kinds of items but these might still create confusions during the
> transition period.
>
> For example, ycmd decided to fallback to None instead of Text when they don't
> know about a symbolkind of a completion item, so users will get to see "File"
> for the include insertions on both folders and files but when they update to
> a newer clangd, they will start to see "File" for files and "None" for
> directory elements. Which I believe might create confusion, but we could
> still fallback to File for those elements(if we handled them within clangd)
> and user experience would neither worsen or improve.
>
> (Currently ycmd's symbolkindcapabilities are actually up-to-date with specs,
> so this issue wouldn't happen. Just wanted to make my point clearer).
Sorry, I don't really understand the question.
Are you talking about the default for `codeActionLiteralSupport`? The protocol
says servers must send `Command`s unless the client indicates support for
`CodeAction`s. There's no room for a different default here.
Or flattening of other properties? That will have no effect on logic, it just
simplifies the code (see D53266).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53213
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
