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