kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land.
LGTM, thanks! ================ Comment at: clangd/ClangdLSPServer.cpp:338 + Command Cmd; + if (Action.command && Action.edit) + return llvm::None; ---------------- sammccall wrote: > 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) Oh I see, nvm then. ================ Comment at: clangd/ClangdLSPServer.cpp:350 + if (Action.kind && *Action.kind == CodeAction::QUICKFIX_KIND) + Cmd.title = "Apply fix: " + Cmd.title; + return Cmd; ---------------- sammccall wrote: > 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...) OK, that makes sense. ================ Comment at: clangd/Protocol.h:390 + /// Flattened from codeAction.codeActionLiteralSupport. + // FIXME: flatten other properties in this way. + bool codeActionLiteralSupport = false; ---------------- sammccall wrote: > 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). > Ok, I thought we were also going to throw away the "valueset"s and just keep whether the client has the capability(and therefore graceful handling) or not. 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
