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.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.

  rG LLVM Github Monorepo


cfe-commits mailing list

Reply via email to