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