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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdServer.cpp:363
+      return CB(A.takeError());
+    return CB((*A)->apply(*Inputs));
+  };
----------------
sammccall wrote:
> we should `format::cleanUpAroundReplacements`... fine to leave this as a FIXME
Added a FIXME. We probably want a helper that does this, to reuse in tests and `ClangdServer`.


================
Comment at: clangd/ClangdServer.h:212
+  /// Apply the code tweak with a specified \p ID.
+  void applyCodeTweak(PathRef File, Range Sel, TweakID ID,
+                      Callback<tooling::Replacements> CB);
----------------
sammccall wrote:
> `tweak`? It's a verb...
That's too short for my taste, I went with `applyTweak` and it also feels helpful if we don't switch between a verb and a noun to keep clear a parallel between a `Tweak` returned by `enumerateTweaks` and a tweak applied in `applyTweak`.

Let me know if you feel strongly about this :-)


================
Comment at: clangd/Protocol.cpp:426
+const llvm::StringLiteral ExecuteCommandParams::CLANGD_APPLY_CODE_ACTION =
+    "clangd.applyCodeAction";
+
----------------
sammccall wrote:
> tweak
Used `applyTweak` for symmetry with `applyFix` (and other reasons mentioned in the comments for `ClangdServer::tweak`)


================
Comment at: clangd/refactor/Tweak.h:40
+  struct Selection {
+    static llvm::Optional<Selection> create(llvm::StringRef File,
+                                            llvm::StringRef Code,
----------------
sammccall wrote:
> sammccall wrote:
> > Not convinced about this helper function.
> >  - much of it is just a passthrough to struct initialization. I think the code calling it would be clearer if it was initialising the fields one-by one
> >  - the only part that's not passthrough is already a function call with a clear name, calling it seems reasonable
> >  - I'm not sure it makes sense to have the Range -> SourceLocation conversion in this file, but the Tweak -> CodeAction conversion outside this file (and not unit-testable). There's an argument to be make to keep this file independent of LSP protocol structs, but I think that argument applies equally to this function.
> Expected<Selection>? Passing an invalid range is always an error I guess.
The reason I added it is to avoid duplication between in the test code and `ClangdServer`, which are the only two clients we have.
I expect this to be more useful when we add a way to traverse the subset of the AST in the checks


================
Comment at: clangd/refactor/Tweak.h:59
+  /// A unique id of the action. The convention is to
+  /// lower-case-with-dashes for the identifier.
+  virtual TweakID id() const = 0;
----------------
sammccall wrote:
> nit: one of my lessons from clang-tidy is that mapping between IDs and implementations is annoying.
> Since IDs are 1:1 with classes, can we just require this to be the class name?
> 
> (If you wanted, I think you could adapt REGISTER_TWEAK so that it goes inside the class defn, and then it could provide the override of id() itself)
That would mean no two tweaks are allowed to have the same class name. This is probably fine, but somewhat contradicts C++, which would solve it with namespaces.
To be fair, there's a simple trick to grep for the id to find its class, so I'd keep as is.

If we choose to adapt `REGISTER_TWEAK`, that would mean we force everyone to put their tweaks **only** in `.cpp` files. That creates arbitrary restrictions on how one should write a check and I'm somewhat opposed to this. But happy to reconsider if you feel strongly about this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56267/new/

https://reviews.llvm.org/D56267