[PATCH] D94126: [ASTMatchers] Make it possible to use empty variadic matchers

[Aaron Ballman via Phabricator via cfe-commits]

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:
> > 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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94126/new/

https://reviews.llvm.org/D94126