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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
