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