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:
> > > > > 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?
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:204
+applyRewriteRule(const ast_matchers::MatchFinder::MatchResult &Result,
+ llvm::ArrayRef<TextChange> Changes);
----------------
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.
================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:164
+ return SmallVector<Transformation, 0>();
+ Transformations.emplace_back(TargetRange, Change.Replacement(Result));
+ }
----------------
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.
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
