ymandel added a comment.

In D61335#1495324 <https://reviews.llvm.org/D61335#1495324>, @ilya-biryukov 
wrote:

> Sorry for the delay.
>  I personally like the `RewriteRule::Action` best. Allows to use a rather 
> common term, while still avoiding any possible confusion.


I tried going with `RewriteRule::Action` (it was my preference as well), but 
found that it didn't work well once I tried folding the composite rules into 
RewriteRule.  The problem is that the Action structure forces you to eagerly 
join the matchers (which is what my first diff did). But then it doesn't work 
to join multiple rules that themselves were built with makeOrderedRule. 
Previously, this was impossible because they had different types, but now a 
user could conceivably do this.  So, I changed the implementation build the 
joined matcher later.  RewriteRule now just stores all of the (sub)rules in a 
single list, and `makeOrderedRule` is a trivial merge.  The real work is done 
at `registerMatchers()`-time where we build the joined matcher for the whole 
rule. But, this means keeping the whole (sub)rule around, not just the Action 
part.

Alternatives I considered:

1. We could use the Action structure inside Transformer -- instead of storing a 
RewriteRule, we could simply store a single matcher and vector of Action. In 
the constructor, we would call `buildMatcher(Rule)` and then copy only the 
action parts from the rules.  However, I don't think this added complexity will 
gain us anything other than a structure which more faithfully reflects the 
control flow -- the matchers in each case are "dead" once we build the joined 
matcher, and the Action approach would reflect that.

2. We could add more structure to Case, like:

  struct RewriteRule {
    struct Action {
      SmallVector<ASTEdit, 1> Edits;
      TextGenerator Explanation;
    };
    struct Case {
      ast_matchers::internal::DynTypedMatcher Matcher;
      Action A;
    };
    std::vector<Cases> Cases;
  };


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to