[PATCH] D53213: [clangd] Send CodeAction responses to textDocument/codeAction (LSP 3.8)

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clangd/ClangdLSPServer.cpp:338
+  Command Cmd;
+  if (Action.command && Action.edit)
+    return llvm::None;
----------------
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.


================
Comment at: clangd/ClangdLSPServer.cpp:350
+  if (Action.kind && *Action.kind == CodeAction::QUICKFIX_KIND)
+    Cmd.title = "Apply fix: " + Cmd.title;
+  return Cmd;
----------------
It seems we only prepend title with Apply fix when we fallback, I believe it would be better to add them in CodeAction instead?


================
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.
----------------
I believe this comment is misleading, do we perform any location check? Maybe change that to say "requested file"?


================
Comment at: clangd/Protocol.h:390
+  /// Flattened from codeAction.codeActionLiteralSupport.
+  // FIXME: flatten other properties in this way.
+  bool codeActionLiteralSupport = false;
----------------
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). 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53213