[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: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