aaron.ballman added inline comments.
================
Comment at: clang/lib/ASTMatchers/ASTMatchersInternal.cpp:402
ArrayRef<DynTypedMatcher> InnerMatchers) {
+ if (InnerMatchers.empty())
+ return true;
----------------
steveire wrote:
> aaron.ballman wrote:
> > steveire wrote:
> > > aaron.ballman wrote:
> > > > Does it make sense to return `true` when there are no inner matchers? I
> > > > would have thought that that if there are no matchers, nothing would
> > > > match (so we'd return `false`)?
> > > When debugging a matcher like
> > >
> > > ```
> > > callExpr(anyOf(
> > > hasType(pointerType()),
> > > callee(functionDecl(hasName("foo")))
> > > ))
> > > ```
> > >
> > > and commenting out inner matchers to get
> > >
> > > ```
> > > callExpr(anyOf(
> > > # hasType(pointerType()),
> > > # callee(functionDecl(hasName("foo")))
> > > ))
> > > ```
> > >
> > > it would be very surprising for this to not match callExprs anymore.
> > On the one hand, I see what you're saying. On the other hand, I think the
> > behavior actually is surprising to some folks (like me). For instance,
> > `std::any_of()` returns `false` when the range is empty, while
> > `std::all_of()` returns `true`.
> >
> > To be conservative with the change, rather than allowing zero matchers with
> > potentially surprising behavior, we could require there be at least one
> > matcher. In that case, if you want to comment out all of the inner
> > matchers, you need to comment out the variadic one as well. e.g.,
> > ```
> > # This is an error
> > callExpr(anyOf(
> > # hasType(pointerType()),
> > # callee(functionDecl(hasName("foo")))
> > ))
> >
> > # Do this instead
> > callExpr(
> > # anyOf(
> > # hasType(pointerType()),
> > # callee(functionDecl(hasName("foo")))
> > # )
> > )
> > ```
> > It's a bit more commenting for the person experimenting, but it's also
> > reduces the chances for surprising behavior. WDYT?
> > On the one hand, I see what you're saying. On the other hand, I think the
> > behavior actually is surprising to some folks (like me). For instance,
> > `std::any_of()` returns `false` when the range is empty, while
> > `std::all_of()` returns `true`.
>
> Yes, I know this diverges from those. However, I think the semantic in this
> patch is the semantic that makes sense for AST Matchers. This patch
> prioritizes usefulness and consistency in the context of writing and
> debugging AST Matchers instead of prioritizing consistency with `std::any_of`
> (which would be non-useful and surprising in the context of debugging an AST
> Matcher).
>
> > if you want to comment out all of the inner matchers, you need to comment
> > out the variadic one as well
>
> Yes, that's what I'm trying to make unnecessary.
>
> > It's a bit more commenting for the person experimenting, but it's also
> > reduces the chances for surprising behavior. WDYT?
>
> I think your suggestion leaves zero-arg `anyOf` matcher inconsistent with
> `allOf` matcher (fails to compile versus does something useful).
>
>
>
> Yes, I know this diverges from those. However, I think the semantic in this
> patch is the semantic that makes sense for AST Matchers. This patch
> prioritizes usefulness and consistency in the context of writing and
> debugging AST Matchers instead of prioritizing consistency with std::any_of
> (which would be non-useful and surprising in the context of debugging an AST
> Matcher).
I'm not suggesting that it needs to be consistent for the sake of consistency,
I'm saying that the behavior you are proposing for the any-of-like matchers
(`anyOf` and `eachOf`) is confusing to me in the presence of no arguments *and*
is inconsistent with other APIs that do set inclusion operations.
> I think your suggestion leaves zero-arg anyOf matcher inconsistent with allOf
> matcher (fails to compile versus does something useful).
Then my preference is for the any-of-like matchers to return `false` when given
zero arguments while the all-of-like matchers return `true`. Testing for
existence requires at least one element while testing for universality does not.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94126/new/
https://reviews.llvm.org/D94126
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
