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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
