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:
> > > 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).
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
