[PATCH] D75365: [AST Matchers] Fix bug in 'optionally' matcher wherein all previous bindings are cleared when all inner matchers fail.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Aaron -- when fixing this bug, I had some other questions about the behavior of this matcher. Specifically, instead of adding bindings to the existing map, it creates a new match result for each matching node (with addMatch).  But, I'm struggling to see how it makes sense for optionally -- why would you want a separate match result for each matching InnerMatch? This behavior seems best reserved for `forEach` and related matchers.

I'd propose (and have a patch ready) to instead accumulate all bindings in the same result, so that `optionally(a, b)` would be equivalent to `allOf(optionally(a), optionally(b))` (which it currently is not).  That is, 'optionally' would always give a single result and each submatcher can add more bindings to the result.

Alternatively, we could restrict `optionally` to take exactly one argument, rather than making it variadic, in which case there's no room for confusion as to its semantics. Multiple optional elements would require using `optionally` within an allof (either explicit or implicit). However, I realize this change is more ambitious and may prevent some valid uses.

WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75365