ilya-biryukov added a comment.
Looks very polished, thanks!
Will have to sink the change in a bit, will come back with more comments on
Monday.
In the meantime, a few initial comments and suggestions.
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:54
+/// boolean expression language for constructing filters.
+class MatchFilter {
+public:
----------------
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.
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:135
+// \endcode
+class RewriteRule {
+public:
----------------
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();
```
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
