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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
