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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 4 inline comments as done.
ymandel added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59376