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:
> > > > 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.
> > 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.
>
> That doesn't achieve the goal I have for this patch.
>
> If that's a blocker, then the only remaining possibility is a different name
> for a matcher which returns true on empty and otherwise behaves like anyOf. I
> doubt a good name for such a thing exists.
>
> So, I'll abandon it for now.
> If that's a blocker, then the only remaining possibility is a different name
> for a matcher which returns true on empty and otherwise behaves like anyOf. I
> doubt a good name for such a thing exists.
I don't think a parallel API would be a good approach. However, I'm happy to
add a few more reviewers to the review to see if more opinions reach a
different conclusion about your proposed changes. If you'd like to go that
route, I'd suggest you add klimek, alexfh, and sammccall when you reopen the
review, and maybe dblaikie and echristo for some variety outside of the usual
AST matcher folks.
FWIW, I found some sources to back my belief that checking existence needs at
least one element
(https://en.wikipedia.org/wiki/Existential_quantification#The_empty_set) while
universality does not
(https://en.wikipedia.org/wiki/Universal_quantification#The_empty_set).
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
