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

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Mar 22 10:13:31 PDT 2019


ilya-biryukov added a comment.

I've just realized the review is probably missing the latest revision.
@ymandel, could you upload the new version?



================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:54
+/// boolean expression language for constructing filters.
+class MatchFilter {
+public:
----------------
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?


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:57
+  using Predicate =
+      std::function<bool(const ast_matchers::MatchFinder::MatchResult &Result)>;
+
----------------
Note: I'm probably not seeing the latest version, so this comment might be outdated. Feel free to ignore if you've already moved this to the matchers.

BTW, I was wondering whether `MatchFilter` carries its weight? What are the pros over: 
```
using MatchFilter = std::function<bool(const ast_matchers::MatchFinder::MatchResult &Result)>;
```

The only thing thats come to mind is that default-constructed function cannot be called, which is less useful than returning true in a default-constructed.
We could handle this in the builder once, the rewrite rule can assert the predicate is always set. 

Do you have other reasons to have it in mind that I'm missing?


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:85
+  Node,
+  /// Given a \c MemberExpr, selects the member's token.
+  Member,
----------------
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.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:89
+  /// relevant name, not including qualifiers.
+  Name,
+};
----------------
Same here, what happens to the template arguments and multi-token names, e.g.
`operator +` or `foo<int, int>`?


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:135
+// \endcode
+class RewriteRule {
+public:
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59376





More information about the cfe-commits mailing list