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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
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
----------------
ymandel wrote:
> 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`?
The reasons I find `TextChange` confusing is because I immediately think of something super-simple (a range of text + the replaced text), and I definitely don't think of the AST.

`SourceChange` and `SourceEdit` does not cause this confusion for me personally, so both look ok. Although they do look pretty similar.
Also, it's not actually a final edit, rather a description of it. So something like `EditDescription` could work too.


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


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:190
 struct Transformation {
+  // Trivial constructor to enable `emplace_back()` and the like.
+  Transformation(CharSourceRange Range, std::string Replacement)
----------------
NIT: I'd suggest just paying a few extra lines for initializing the struct instead of using the ctor.
```
Transformation T;
T.Range = ...;
T.Replacement = ...;

v.push_back(std::move(T));
```
or 
```
v.emplace_back();
v.back().Range = ...;
v.back().Replacement = ...;
```

But I can see why you might want a ctor instead. If you decide to leave it, consider re-adding the default ctor that got implicitly deleted as you defined this other one.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:204
+applyRewriteRule(const ast_matchers::MatchFinder::MatchResult &Result,
+                 llvm::ArrayRef<TextChange> Changes);
 
----------------
Why would we change the interface here? Rewrite rule seemed like a perfect input to this function


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:164
+      return SmallVector<Transformation, 0>();
+    Transformations.emplace_back(TargetRange, Change.Replacement(Result));
+  }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60408