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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
