aaron.ballman added a comment.
In D88275#2303283 <https://reviews.llvm.org/D88275#2303283>, @ymandel wrote:
>> I'm not concerned about the basic idea behind the proposed matcher, I'm only
>> worried we're making AST matching more confusing by having two different
>> ways of inconsistently accomplishing the same-ish thing.
>
> Aaron, I appreciate this concern, but I would argue that this matcher isn't
> making things any worse. We already have the various `ignoringImplicit`
> matchers, and this new one simply parallels those, but for parents. So, it is
> in some sense "completing" an existing API, which together is an alternative
> to `traverse`.
I'm not certain I agree with that reasoning because you can extend it to
literally any match that may interact with implicit nodes, which is the whole
point to the spelled in source traversal mode. I'm not certain it's a good
design for us to continue to add one-off ways to ignore implicit nodes.
> We should decide our general policy on these implict matchers vs the traverse
> matchers.
I agree.
> Personally, I view `traverse` as an experimental feature that we're still
> figuring out and so would prefer that it not block progress on new matchers.
> But, I'm open to discussion. I can implement this one in my own codebase in
> the meantime if this requires longer discussion (that is, it's not blocking
> me, fwiw).
FWIW, I think of `traverse` as experimental as well. I can see the argument for
not letting an experimental feature block progress on new matchers, too. I'm
mostly worried about the case where we add the new matches and keep the
`traverse` API, but they have subtly different behaviors and no clear policy on
when to use which form.
> Also, I don't believe that traverse work in this case. When I change the test
> to use `traverse`, it fails:
>
> TEST(HasParentIgnoringImplicit, TraverseMatchesExplicitParents) {
> std::string Input = R"cc(
> float f() {
> int x = 3;
> int y = 3.0;
> return y;
> }
> )cc";
> // ---> Passes because there are no implicit parents.
> EXPECT_TRUE(matches(
> Input, integerLiteral(traverse(TK_IgnoreImplicitCastsAndParentheses,
> hasParent(varDecl())))));
> // Fails
> EXPECT_TRUE(
> matches(Input,
> declRefExpr(traverse(TK_IgnoreImplicitCastsAndParentheses,
> hasParent(returnStmt())))));
> // Fails
> EXPECT_TRUE(
> matches(Input,
> floatLiteral(traverse(TK_IgnoreImplicitCastsAndParentheses,
> hasParent(varDecl())))));
> }
Interesting catch and not the behavior I would expect from `traverse`.
@steveire, would you consider this to be a bug in the traversal behavior?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88275/new/
https://reviews.llvm.org/D88275
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
