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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

Patch LGTM aside from a formatting nit.

In D75365#1898466 <https://reviews.llvm.org/D75365#1898466>, @ymandel wrote:

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


Agreed, that does seem a bit strange.

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

Good question! My intuition is that `optionally` should take exactly one argument and the user should be explicit as to whether they mean `allOf` or `anyOf` when there is a list of optional matchers. Defaulting to the `allOf` behavior for a list may be surprising to some folks because the `anyOf` behavior also seems reasonable for a list of optional matchers. WDYT?



================
Comment at: clang/lib/ASTMatchers/ASTMatchersInternal.cpp:353
     BoundNodesTreeBuilder BuilderInner(*Builder);
-    if (InnerMatcher.matches(DynNode, Finder, &BuilderInner))
+    if (InnerMatcher.matches(DynNode, Finder, &BuilderInner)){
+      Matched = true;
----------------
Please fix the formatting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75365