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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
