[PATCH] D94879: Implement dynamic mapAnyOf in terms of ASTNodeKinds

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:948
+          return {};
+        auto VM = Arg.Value.getMatcher();
+        if (VM.hasTypedMatcher(NK)) {
----------------
Note, this is an example of why use of `auto` is discouraged in the project when the type is not spelled out explicitly in the initialization -- this was accidentally creating a non-const copy of the `VariantMatcher`, not getting a const reference as `getMatcher()` returns. Not the worst problem in the world, but it takes a lot of review effort to find these issues when you use `auto`, which is why the guidance is what it is.


================
Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:950
+        if (VM.hasTypedMatcher(NK)) {
+          auto DM = VM.getTypedMatcher(NK);
+          InnerArgs.push_back(DM);
----------------
Rather than have a has/get pair, would it make sense to have a get method that returns an `Optional` so that we don't have to check the same thing twice?


================
Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:988
+      *LeastDerivedKind = CladeNodeKind;
+    return true;
   }
----------------
We used to traverse the list of matcher functions to see if one is convertible or not and now we're always assuming that the conversion is fine -- was that previous checking unnecessary or has it been subsumed by checking somewhere else?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94879