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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Can live with `applyTweak` etc, though it makes me sad.



================
Comment at: clangd/refactor/Tweak.h:40
+  struct Selection {
+    static llvm::Optional<Selection> create(llvm::StringRef File,
+                                            llvm::StringRef Code,
----------------
ilya-biryukov wrote:
> 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
I understand. I think as things stand both callers would be clearer (if a couple of lines longer) without this helper.

What the API should be in the future - happy to talk about that then.


================
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;
----------------
ilya-biryukov wrote:
> 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.
I don't care about the details (e.g. whether `REGISTER_TWEAK` sets the name, asserts the name, or none of the above).

I do care that we don't add a second ID for a class that's not equal to the class name. This is both a bad idea from first principles and from experience with clang-tidy.
If this were ever to become a real problem, I'm happy to include the namespace name in the ID.


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

https://reviews.llvm.org/D56267