[PATCH] D56267: [clangd] Interfaces for writing code actions

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jan 17 09:37:44 PST 2019


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





More information about the cfe-commits mailing list