ymandel marked 4 inline comments as done.
ymandel added inline comments.
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:54
+/// boolean expression language for constructing filters.
+class MatchFilter {
+public:
----------------
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.
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:135
+// \endcode
+class RewriteRule {
+public:
----------------
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.
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
