[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 10 inline comments as done.
ymandel added a comment.

Addressed the most major comments. Still working on some smaller ones. PTAL.  Thanks!



================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:54
+/// boolean expression language for constructing filters.
+class MatchFilter {
+public:
----------------
ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > Intuitively, it feels that any filtering should be possible at the level of the AST matchers. Is that not the case?
> > > Could you provide some motivating examples where AST matchers cannot be used to nail down the matching nodes and we need `MatchFilter`? 
> > > 
> > > Please note I have limited experience with AST matchers, so there might be some obvious things that I'm missing.
> > Good point. The examples I have would actually be perfectly suited to a matcher.  That said, there is not matcher support for a simple predicate of this form, along the lines of gtest's `Truly(predicate)`. I'll remove this and separately try to add something like `Truly` to the matchers.
> Makes sense! Maybe put a `FIXME` here to let the readers know this is moving to the ast matchers?
I've removed the where clause entirely. I'll separately add the matcher support (I've figured out how to do it within the existing framework).


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:85
+  Node,
+  /// Given a \c MemberExpr, selects the member's token.
+  Member,
----------------
ilya-biryukov wrote:
> What happens if member is composed of multiple tokens? E.g.
> ```
> foo.bar::baz; // member tokens are ['bar', '::', 'baz']
> foo.template baz<int>; // member tokens are ['template', 'baz', '<', 'int', '>']
> foo.operator +; // member tokens are ['operator', '+']
> ```
> 
> I might be misinterpreting the meaning of "member" token.
Good catch! I've updated to handle these correctly (I believe). Added some tests, plan to add some more.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:89
+  /// relevant name, not including qualifiers.
+  Name,
+};
----------------
ilya-biryukov wrote:
> Same here, what happens to the template arguments and multi-token names, e.g.
> `operator +` or `foo<int, int>`?
Good point. This seems difficult to get right, since NamedDecl does not carry sufficient loc data.  However, I've updated the code to explicitly fail in such cases, so at least we won't have bad rewrites.

BTW, any idea whether constructor initializers can ever be multi token?


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:135
+// \endcode
+class RewriteRule {
+public:
----------------
ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > ymandel wrote:
> > > > ilya-biryukov wrote:
> > > > > Maybe consider separating the fluent API to build the rewrite rule from the rewrite rule itself?
> > > > > 
> > > > > Not opposed to having the fluent builder API, this does look nice and seems to nicely align with the matcher APIs.
> > > > > However, it is somewhat hard to figure out what can `RewriteRule` do **after** it was built when looking at the code right now.
> > > > > ```
> > > > > class RewriteRule {
> > > > > public:
> > > > >   RewriteRule(DynTypedMatcher, TextGenerator Replacement, TextGenerator Explanation);
> > > > > 
> > > > >   DynTypedMatcher matcher() const;
> > > > >   Expected<string> replacement() const;
> > > > >   Expected<string> explanation() const;
> > > > > };
> > > > > 
> > > > > struct RewriteRuleBuilder { // Having a better name than 'Builder' would be nice.
> > > > >   RewriteRule finish() &&; // produce the final RewriteRule.
> > > > > 
> > > > >   template <typename T>
> > > > >   RewriteRuleBuilder &change(const TypedNodeId<T> &Target,
> > > > >                       NodePart Part = NodePart::Node) &;
> > > > >   RewriteRuleBuilder &replaceWith(TextGenerator Replacement) &;
> > > > >   RewriteRuleBuilder &because(TextGenerator Explanation) &;
> > > > > private:
> > > > >   RewriteRule RuleInProgress;
> > > > > };
> > > > > RewriteRuleBuilder makeRewriteRule();
> > > > > ```
> > > > I see your point, but do you think it might be enough to improve the comments on the class? My concern with a builder is the mental burden on the user of another concept (the builder) and the need for an extra `.build()` everywhere. To a lesser extent, I also don't love the cost of an extra copy, although I doubt it matters and I suppose we could support moves in the build method.
> > > The issues with the builder interface is that it requires lots of boilerplate, which is hard to throw away when reading the code of the class. I agree that having a separate builder class is also annoying (more concepts, etc).
> > > 
> > > Keeping them separate would be my personal preference, but if you'd prefer to keep it in the same class than maybe move the non-builder pieces to the top of the class and separate the builder methods with a comment. 
> > > WDYT? 
> > > 
> > > > To a lesser extent, I also don't love the cost of an extra copy, although I doubt it matters and I suppose we could support moves in the build method.
> > > I believe we can be as efficient (up to an extra move) with builders as without them. If most usages are of the form `RewriteRule R = rewriteRule(...).change(...).replaceWith(...).because(...);`
> > > Then we could make all builder functions return r-value reference to a builder and have an implicit conversion operator that would consume the builder, producing the final `RewriteRule`:
> > > ```
> > > class RewriteRuleBuilder {
> > >   operator RewriteRule () &&;
> > >   /// ...
> > > };
> > > RewriteRuleBuilder rewriteRule();
> > > 
> > > void addRule(RewriteRule R);
> > > void clientCode() {
> > >   addRule(rewriteRule().change(Matcher).replaceWith("foo").because("bar"));
> > > }
> > > ```
> > I didn't realize that implicit conversion functions are allowed. With that, I'm fine w/ splitting.   Thanks!
> Have you uploaded the new version? I don't seem to see the split.
I have now.  FWIW, I've left both ref overloads, but what do you think of dropping the lvalue-ref overloads and only supporting rvalue refs?  Users can still pretty easily use an lvalue, just by inserting a trivial std::move() around the lvalue.  


================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:157
+      .as<clang::Expr>()
+      .replaceWith(text("REPLACED"))
+      .because(text("Use size() method directly on string."));
----------------
ilya-biryukov wrote:
> NIT: maybe consider adding overloads for these function that allow to pass char literals there? This is probably a common case, so it'd be nice to write this as:
> ```
> replaceWith("REPLACED").because("Use size() method directly on string.");
> ```
Sure, will do in next revision.  The version that's integrated w/ stencils does exactly this, btw.  I wonder, though, whether we should add an implicit constructor to the TextGenerator class instead of adding overloads.  I have a general distrust of implicit constructors, but I see that's not the case in the clang codebase. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59376