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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as not done.
ilya-biryukov added inline comments.


================
Comment at: clangd/refactor/Tweak.h:40
+  struct Selection {
+    static llvm::Optional<Selection> create(llvm::StringRef File,
+                                            llvm::StringRef Code,
----------------
sammccall wrote:
> 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.
I'd keep this function, here is my reasoning:

1. There are 4 usages already (2 in this patch, 2 in the tests), keeping them in sync means an annoying (to my taste) amount of copy-paste.
2. This function is not completely trivial, as it makes a decision on what a `CursorLoc` is and documents it.
3. Making changes is simpler: e.g. if we add more fields to `Tweak::Selection`, we'd end up changing this function (possibly its signature too) and all of its callers. There's almost no room for error. If we remove it, one would have to find and update all usages by hand and make sure they're the same.

I did update the patch to inline it, but let me know if any of this convinces you.



================
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:
> 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.
Done, `REGISTER_TWEAK` now defines `id`.

Out of curiosity, what are the problems with clang-tidy checks that you mention? Finding a check by its name being hard is something that comes to mind. Anything else?


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

https://reviews.llvm.org/D56267