[PATCH] D54404: Exclude matchers which can have multiple results

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: sbenza.
aaron.ballman added a subscriber: sbenza.
aaron.ballman added a comment.

In https://reviews.llvm.org/D54404#1295426, @steveire wrote:

> I acknowledge and share the future-proofing concern.
>
> We could possibly use something trait-based instead and put the trait beside the matcher definition in ASTMatchers.h, but that doesn't really solve the problem. It only moves the problem.


If we could somehow incorporate it into the matcher definition itself, though, it means we don't have two lists of things to keep updated. You're right that it doesn't eliminate the problem -- we still have to know to ask the question when reviewing new matchers (so perhaps something that requires you to explicitly opt-in/out would be better).

Adding @sbenza in case he has ideas.



================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:604
 
+  static std::vector<StringRef> excludedMatchers{
+      "allOf",
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > `excludedMatchers` -> `ExcludedMatchers`
> Please add comments that explain why these are excluded and under what circumstances this list should be updated to add or remove items.
> 
> Also: how do we ensure this list stays properly updated? We sometimes miss updating `RegistryMaps()`, so I'm a bit concerned that adding a second list in a different location will be inviting trouble.
The comment is a bit better but still not enough to tell me what's common to all of these matchers. It sort of reads to me like "all the matchers with overloads" since overloaded matchers can be ambiguous with particular arguments, but that's not what I think you're going for here.

For instance, I don't see how the `anything()` matcher can be ambiguous when used with particular arguments -- it doesn't accept arguments and it matches literally everything.


Repository:
  rC Clang

https://reviews.llvm.org/D54404