ymandel marked an inline comment as done.
ymandel added inline comments.
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:87
+ TextGenerator Replacement;
+ TextGenerator Explanation;
+};
----------------
ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > ymandel wrote:
> > > > ilya-biryukov wrote:
> > > > > ymandel wrote:
> > > > > > ilya-biryukov wrote:
> > > > > > > I would've expected explanation to be the trait of the rewrite
> > > > > > > rule, since all changes have to be applied.
> > > > > > > What's the reasoning behind having it at the level of separate
> > > > > > > changes? How would this explanation be used? For debugging
> > > > > > > purposes or displaying that to the user?
> > > > > > I think that for most cases, one explanation sufficies for the
> > > > > > whole transformation. However, there are some tidies that emit
> > > > > > multiple diagnoses (for example if changing before a declaration
> > > > > > and a definition). Would it help if I clarify in the comments?
> > > > > Yeah, absolutely! Please document what it's used for and that would
> > > > > clear that up for me.
> > > > > I actually thing that explaining every part of the transformation is
> > > > > probably too complicated, so most of the time you would want to have
> > > > > an explanation for the `RewriteRule`, not for each individual change.
> > > > >
> > > > > The other challenge that I see is show to display these explanations
> > > > > to the user, i.e. how should the clients combine these explanations
> > > > > in order to get the full one? Should the `RewriteRule` have an
> > > > > explanation of the full transformation too?
> > > > I've revised the comments, changed the name to "Note" and put
> > > > "Explanation" back into RewriteRule.
> > > >
> > > > As for how to display these, I imagine an interface like clang tidy's
> > > > fixit hints. The Explanation (if any) will be associated with the span
> > > > of the entire match. The Notes will be associated with the target node
> > > > span of each annotated change. WDYT?
> > > Do we really need the AST information to expose the edits to the users?
> > > IIUC, clang-tidy uses the information from textual replacements to render
> > > the changes produced by the fix-its today.
> > >
> > > I guess it might be useful to add extra notes to clang-tidy warnings that
> > > have corresponding fix-its, but is the transformers library the right
> > > layer to produce those?
> > > I haven't seen the proposed glue to clang-tidy yet, maybe that would make
> > > more sense when I see it.
> > >
> > > One of the other reasons I ask this is that it seems that without `Note`
> > > we don't strictly `ASTEditBuilder`, we could replace
> > > ```
> > > change("ref").to("something"); // without nodepart
> > > change("ref", NodePart::Member).to("something");
> > > ```
> > > with
> > > ```
> > > change("ref", "something")
> > > change("ref", NodePart::Member, "something");
> > > ```
> > > That would remove the boilerplate of the builder, simplifying the code a
> > > bit.
> > >
> > > That trick would be hard to pull if we have a `Note` field inside, we'll
> > > need more overloads and having note and replacement after each other
> > > might be confusing (they have the same type, might be hard to read
> > > without the guiding `.to()` and `.because()`)
> > Breaking this explicitly into two questions:
> > 1. Do Notes belong here?
> > 2. Can we drop the builder?
> >
> > 1. Do Notes belong here?
> > I think so. When users provide a diagnostic, they are specifying its
> > location. So, we don't quite need the AST but we do need location info.
> > The diagnostic generator does take information from the replacements
> > themselves, but that's not alone. For example:
> > clang-tidy/readability/ConstReturnTypeCheck.cpp:104. That demos both the
> > diagnostic construction and multiple diagnostics in a single tidy result.
> >
> > Given that, i think that RewriteRules are the appropriate place. The goal
> > is that they be self contained, so I think the explanations should be
> > bundled with the description of that actual change. An example approach to
> > merging with clang tidy is here:
> > https://github.com/ymand/llvm-project/blob/transformer/clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp
> > (although that's based on a somewhat different version of the Transformer
> > library; it should make clear what I have in mind).
> >
> > 2. Do we need the builder?
> > No, despite my opinion that notes belong. Given the explanation on
> > RewriteRule, I think notes will be used only rarely (like in the example
> > clang-tidy above; but that's not standard practice). So, we can provide
> > the two overloads of `change` that you mention and then let users assign
> > the note directly in those rare cases they need it. then, we can drop the
> > builder.
> >
> > So, I propose keeping the note but dropping the builder. WDYT?
> Thanks for pointing out how this usage in clang-tidy. Seems to be the only
> way to put it into clang-tidy (and there are good reasons for that, e.g.
> having the full context of the change).
>
> > So, I propose keeping the note but dropping the builder. WDYT?
> LGTM!
>
>
Removed the builder, added more `change` overloads. Also renamed `changeRoot`
to `changeMatched` because I think it's a better description.
The only part I don't love (here and elsewhere) is the duplicate overloads one
each for `std::string` and `TextGenerator`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60408/new/
https://reviews.llvm.org/D60408
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
