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

Fri Apr 12 07:38:53 PDT 2019

ymandel added a comment.

In D60408#1463909 <https://reviews.llvm.org/D60408#1463909>, @ilya-biryukov wrote:

> Sorry for the delay.
>  There seem to be a few changes that are unrelated to the actual patch. Could we separate various non-functional changes (moving code around, etc.) into a separate change to keep the diff for this one minimal?

I've done that as far as I can tell. Please let me know if I've missed anything.

================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:80
+// \endcode
+struct TextChange {
+  // The (bound) id of the node whose source will be replaced.  This id should
----------------
ilya-biryukov wrote:
> `MatchChange` or something similar might be a better name.
> This actually tries to change the matched AST node to a textual replacement.
I chose this to contrast with an AST change.  The idea being that we're specifying a replacement relative to source code locations (informed by the ast). If we later, say, integrate with your library I could imagine specifying changes to AST nodes.  But, maybe I'm overthinking... If we're going to drop "text", what about "source?" be clearer than "text"? E.g, `SourceChange` or (my preference) `SourceEdit`?

================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:87
+  TextGenerator Replacement;
+  TextGenerator Explanation;
+};
----------------
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?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60408/new/

https://reviews.llvm.org/D60408