steveire abandoned this revision.
steveire added inline comments.
================
Comment at: clang/lib/ASTMatchers/ASTMatchersInternal.cpp:402
ArrayRef<DynTypedMatcher> InnerMatchers) {
+ if (InnerMatchers.empty())
+ return true;
----------------
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.
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
