ilya-biryukov added inline comments.
================
Comment at: clangd/ClangdLSPServer.cpp:346
+ {ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND,
+ ExecuteCommandParams::CLANGD_APPLY_CODE_ACTION}},
}},
----------------
sammccall wrote:
> Seems a bit more direct to give each one its own command name? Maybe under a
> namespace like refactor/swapIfBranches
I'm not sure what we're winning here.
Currently we have: `command:{ name: "applyCodeAction", args:
["swap-if-branches", "…"] }`
In the proposed approach we'd have: `command: { name: "swap-if-branches", args:
[…] }`
The latter would definitely mean more fiddling at least to communicate the list
of supported actions. Both approaches don't seem to require any changes on the
client side, even though they affect the messages we send over the protocol.
================
Comment at: clangd/ClangdLSPServer.cpp:662
+ std::vector<CodeAction> Actions = std::move(FixIts);
+ auto toCodeAction = [&](PreparedAction &&A) -> CodeAction {
+ CodeAction CA;
----------------
sammccall wrote:
> This seems like it could just be a helper function... what does it capture?
> I think it might belong next to PreparedActions just like we have
> `toLSPDiags` in `Diag.h` - it's not ideal dependency-wise, but inlining
> everything into ClangdLSPServer seems dubious too. Happy to discuss offline...
I've moved it into a helper function, moved away from using a lambda.
However, kept it in `ClangdLSPServer.cpp`, `Diag.h` does feel like the wrong
place for it, it would be nice to find a better place for both functions.
================
Comment at: clangd/refactor/Tweak.cpp:62
+ }
+ // Ensure deterministic order of the results.
+ llvm::sort(Available,
----------------
sammccall wrote:
> (nit: I'd make this `operator<` of `Tweak`? e.g. if priority is added, it
> should be private)
My personal preference would be to keep it in .cpp file to avoid complicating
the public interface with a function and a friend declaration for
`prepareTweaks` (let me know if I misinterpreted your proposal).
Let me know if you have strong preference wrt to this one
================
Comment at: clangd/refactor/Tweak.h:78
+/// Contains all actions supported by clangd.
+typedef llvm::Registry<Tweak> TweakRegistry;
+
----------------
sammccall wrote:
> nit: can we move this to the cpp file and out of the public interface?
> (or keep the registry public, but drop prepareTweak[s] and just make callers
> do it)
Done.
I don't think that makes a big difference, though: hiding the typedef would
hide the fact we're using the `Registry`, though. We still have
`REGISTER_TWEAK` that mentions it in the public interface.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
