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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
