ilya-biryukov added a comment.
A few quick responses, will take a closer look again tomorrow.
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:87
+ TextGenerator Replacement;
+ TextGenerator Explanation;
+};
----------------
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!
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:204
+applyRewriteRule(const ast_matchers::MatchFinder::MatchResult &Result,
+ llvm::ArrayRef<TextChange> Changes);
----------------
ymandel wrote:
> ilya-biryukov wrote:
> > ymandel wrote:
> > > ilya-biryukov wrote:
> > > > Why would we change the interface here? Rewrite rule seemed like a
> > > > perfect input to this function
> > > Good point. For the name, you're right -- but I think the name was
> > > misleading. The function doesn't really "apply" anything and what it does
> > > use is only the _change_ part of the rule -- it doesn't handle (and never
> > > even looks at) the matcher part, because it's already given the match.
> > >
> > > Rather, this just translates change descriptions that are in terms of the
> > > AST into ones in terms of the source code. I've renamed it to
> > > `translateChanges` and updates the comments describing it. Let me know
> > > what you think.
> > The new name looks ok. Although if the `RewriteRule` is the central concept
> > of this file (and it looks like it is), I think it'd make sense to make
> > this a narrower interface, accepting a `RewriteRule` as a parameter.
> >
> > But up to you, as long as we have this spelled out in the docs, I don't
> > think this should cause problems to the users.
> SG. To be clear -- this is intended only as an "advanced" part of the API,
> to factor out code that will be common to:
> 1. Transformer (here)
> 2. TransformerTidy -- the clang-tidy version of this code that creates a
> ClangTidy from a RewriteRule
> 3. applyRewriteRule(ASTContext) -- an upcoming function that will do what
> this function looked like it was doing. It will apply a rewriterule
> (including the matcher) to a node or an ASTContext.
>
> However, I don't expect that any standard users should need this. I'd
> originally placed it in an "internal" namespace, but it's not quite internal
> because TransformerTidy will be living somewhere else. So, it should be
> exposed, but its really only for other implementors rather than standard
> users.
The tidy use-case makes sense now, thanks! The fact that fix-it hints is the
only way to report changes there kinda forces us to have a function that works
on a level of transformations.
================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:164
+ return SmallVector<Transformation, 0>();
+ Transformations.emplace_back(TargetRange, Change.Replacement(Result));
+ }
----------------
ymandel wrote:
> ilya-biryukov wrote:
> > ymandel wrote:
> > > ilya-biryukov wrote:
> > > > What if the changes intersect? I'd expect this function to handle this
> > > > somehow, handling this is complicated and I feel we shouldn't rely on
> > > > the clients to make it fast.
> > > Right now, we rely on clients (as you mention). The clients will receive
> > > `AtomicChange`s that overlap and will have to handle that.
> > > `clang::tooling::applyAtomicChanges` will fail on this, although one
> > > could imagine a client that could be smarter.
> > >
> > > That said, I'm fine with us handling this here, instead. My only argument
> > > against is that the same issue applies for matchers in general, so
> > > catching it here won't prevent the case where separate firings of the
> > > same rule (or of different rules if more than one is registered) try to
> > > change the source. Moreover, since we already have the logic elsewhere
> > > (clang::tooling::Replacements) we would be duplicating that here.
> > >
> > > I've thought of using AtomicChange here directly, but there's no way to
> > > get the changes back in terms of SourceLocations, which we need for this
> > > function to be useable inside the ClangTidy adapter (FixItHint uses
> > > SourceLocations, not raw filename and offset).
> > >
> > > What would you say to a FIXME to find a better means of failing fast?
> > >
> > > On a related note, I see that this struct is very similar to
> > > clang::FixItHint. Do you think its worth just using that directly? Or,
> > > is this a sufficiently different situation to merit a different type
> > > definition?
> > I can hardly imagine clients doing anything smarter than failing on
> > intersecting changes (at least for the general case).
> >
> > That said, I do see the reasons to keep this error-handling out of this
> > function for the matter of simplicity. And totally agree that in order to
> > deal with it properly we'd want to return something like
> > `tooling::Replacements` to make sure we can reuse facilities that handle
> > the overlapping cases properly.
> >
> > Could we add a comment to the declaration, explaining that returned ranges
> > might overlap and clients are expected to deal with that (and maybe
> > mentioning `AtomicChanges` and `tooling::Replacements` as potential options
> > to do so).
> >
> > I'd avoid using `clang::FixItHint` while we keep experimenting with the
> > interface of the library, having our own type gives us more freedom to
> > change it. As soon as the interface is stable, we can take revisit this
> > (and FWIW, `FixItHint` does not look like a good name for this).
> Agreed. I've updated the comments as per your suggestion. Also added a note
> to the Transformer constructor below along the same lines.
>
> For the future: we should think about adding an option to have Transformer
> handle conflicts instead of leaving it up to the user. It would probably
> require a somewhat different API -- e.g. creating one large AtomicChange and
> handing that off at destruction time, rather then calling `Consumer` with
> independent changes for each match.
Thanks, LG!
Agree that having a simpler interface would be nice, still not sure how to
properly wire it up with matchers, especially in the clang-tidy use-case.
But if we can run the library function would run the matchers on its own, a
simpler interface seems doable.
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
