[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
Wed Mar 20 10:22:11 PDT 2019


ilya-biryukov added inline comments.


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


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