[PATCH] D65877: [libTooling] In Transformer, generalize `applyFirst` to admit rules with incompatible matchers.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 2 inline comments as done.
ymandel added a comment.

Thanks for your detailed and helpful feedback.



================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:129
+  Buckets.back().Cases.emplace_back(CaseId, std::move(Case));
 }
 
----------------
gribozavr wrote:
> Would it be easier to implement this algorithm using `ASTNodeKind::getMostDerivedCommonAncestor` instead of `isBaseOf`?
> 
> Go over all buckets, for each bucket check if common ancestor between the bucket kind and the new case kind is not none. If it is not none, insert the new case into that bucket, and update bucket's kind to the new common ancestor kind.
> 
> You could also go as far as setting the bucket's kind to always the the root of the corresponding kind hierarchy (TemplateArgument, TemplateName, Decl, Stmt etc.), since given enough diverse cases, the bucket kind will converge to the root kind.
Based on our discussion, I've drastically simplified the code. As per our discussion: since each matcher must be a node matcher, we only have the root AST types to consider. Therefore, there's no need for comparison of relative levels in the kind hierarchy -- we simply map each matcher to a root kind and then put it in that bucket.  This lets us remove lots of custom code.


================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:520
            Expected);
 }
 
----------------
gribozavr wrote:
> If this test and the test below are testing rule ordering only, I find them to have too much unrelated detail. We could do something simpler and more self-contained like:
> 
> input:
> ```
> void f1();
> void f2();
> void call_f1() { f1(); }
> void call_f2() { f2(); }
> ```
> 
> ```
> replaceF1 = makeRule(
>   callExpr(callee(functionDecl(hasName("f1"))),
>   change(text("REPLACE_F1"))
> )
> 
> replaceF1orF2 = makeRule(
>   callExpr(callee(functionDecl(hasAnyName("f1", "f2"))),
>   change(text("REPLACE_F1_OR_F2"))
> )
> ```
> 
> ```
> applyFirst({replaceF1, replaceF1orF2})
> => check that the output is =>
> void f1();
> void f2();
> void call_f1() { REPLACE_F1; }
> void call_f2() { REPLACE_F1_OR_F2; }
> ```
> 
> ```
> applyFirst({replaceF1orF2, replaceF1})
> => check that the output is =>
> void f1();
> void f2();
> void call_f1() { REPLACE_F1_OR_F2; }
> void call_f2() { REPLACE_F1_OR_F2; }
> ```
> 
> Up to you.
Much nicer. Thanks!


================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:582
+  EXPECT_EQ(tooling::detail::buildMatchers(Rule).size(), 2UL);
+  testRule(Rule, Input, Expected);
+}
----------------
gribozavr wrote:
> Ditto, I think we can make a simpler and more self-contained test:
> 
> input:
> ```
> void f1();
> void f2();
> void call_f1() { f1(); }
> ```
> 
> ```
> stmtRule = makeRule(
>   callExpr(callee(functionDecl(hasName("f1"))),
>   change(text("STMT_RULE")))
> 
> declRule = makeRule(
>   functionDecl(hasName("f2")).bind("fun"),
>   change(name("fun"), text("DECL_RULE")))
> 
> rule = applyFirst({stmtRule, declRule})
> ```
> 
> expected output:
> ```
> void f1();
> void DECL_RULE();
> void call_f1() { STMT_RULE(); }
> ```
I kept a little of the complexity of the old test -- I still have a third rule with the same kind as the first to make sure rules are bucketed together even when separated by different rule.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65877