[PATCH] D60408: [LibTooling] Extend Transformer to support multiple simultaneous changes.

Mon Apr 15 12:28:05 PDT 2019

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