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

Yitzhak Mandelbaum via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Apr 12 13:01:27 PDT 2019


ymandel 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
----------------
ilya-biryukov wrote:
> 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.
Right, I'm now tending way from the whole focus on text/source/etc. I agree with your point that this is operating at the AST level, especially in light of the discussion below on applyRewriteRule.  Given that these are all anchored by an AST node, let's go with `ASTEdit`, unless that will conflict with the library that you're developing?

I agree that "Description" is more apt, but I feel like this is (somewhat) implicit in the fact that its a struct versus a function (which would actually be carrying out the action). I'm also afraid that "EditDescription" will read like an action, which may be a bit confusing (although the casing should help distinguish).


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


================
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)
----------------
ilya-biryukov wrote:
> 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.
Agreed. I only use it one place, hardly worth the cost. thanks


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:204
+applyRewriteRule(const ast_matchers::MatchFinder::MatchResult &Result,
+                 llvm::ArrayRef<TextChange> Changes);
 
----------------
ilya-biryukov wrote:
> Why would we change the interface here? Rewrite rule seemed like a perfect input to this function
Good point.  For the name, you're right -- but I think the name was misleading. The function doesn't really "apply" anything and what it does use is only the _change_ part of the rule -- it doesn't handle (and never even looks at) the matcher part, because it's already given the match.

Rather, this just translates change descriptions that are in terms of the AST into ones in terms of the source code. I've renamed it to `translateChanges` and updates the comments describing it. Let me know what you think.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:164
+      return SmallVector<Transformation, 0>();
+    Transformations.emplace_back(TargetRange, Change.Replacement(Result));
+  }
----------------
ilya-biryukov wrote:
> 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.
Right now, we rely on clients (as you mention).  The clients will receive `AtomicChange`s that overlap and will have to handle that.  `clang::tooling::applyAtomicChanges` will fail on this, although one could imagine a client that could be smarter.

That said, I'm fine with us handling this here, instead. My only argument against is that the same issue applies for matchers in general, so catching it here won't prevent the case where separate firings of the same rule (or of different rules if more than one is registered) try to change the source.  Moreover, since we already have the logic elsewhere (clang::tooling::Replacements) we would be duplicating that here.

I've thought of using AtomicChange here directly, but there's no way to get the changes back in terms of SourceLocations, which we need for this function to be useable inside the ClangTidy adapter (FixItHint uses SourceLocations, not raw filename and offset).

What would you say to a FIXME to find a better means of failing fast?

On a related note, I see that this struct is very similar to clang::FixItHint.  Do you think its worth just using that directly? Or, is this a sufficiently different situation to merit a different type definition?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60408





More information about the cfe-commits mailing list