Implement dynamic mapAnyOf in terms of ASTNodeKinds

[steveire]

Summary

This reduces template bloat, but more importantly, makes it possible to
construct one from clang-query without template types.

[aaron.ballman]

clang/lib/ASTMatchers/Dynamic/Marshallers.h

948

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.

950

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?

988

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?

[steveire]

Update

[steveire]

clang/lib/ASTMatchers/Dynamic/Marshallers.h

948

Note, this is an example of why use of auto is discouraged

Nope, this isn't a good example of that. It's actually the opposite. auto does no harm here.

If I had written

VariantMatcher VM = Arg.Value.getMatcher();

you would either already-know what the return type of getMatcher() is and see the copy, or you would be satisfied that the variable type is not auto (dangerously, potentially) and move on, or you would go and check the return type of getMatcher() if you had a suspicion.

If I had written

SomeTypedefName VM = Arg.Value.getMatcher();

you wouldn't see an auto, which again might be satisfying, but you would have to go and look at the typedef to see if it contains a const or a & (for example see ValueParamT in SmallVector which is either const T& or T, depending on T).

Requiring non-use of auto is not a way around knowing or checking the return value of methods, and can actually give you a false sense of security!

I don't think you'll ever convince me that your way doesn't make the code worse :).

950

I think you're talking about the VariantMatcher API, so I think that's out of scope of this patch.

988

Yes, the checking was not necessary. Because this matcher is basically a convenient implementation of stmt(anyOf(ifStmt(innerMatchers), forStmt(innerMatchers))), it's the outer stmt that it is convertible to.

[aaron.ballman]

LGTM!

clang/lib/ASTMatchers/Dynamic/Marshallers.h

948

Nope, this isn't a good example of that. It's actually the opposite. auto does no harm here.

Here's a simplified example showing what I meant: https://godbolt.org/z/svbx4f. Note how the use with just auto executes the copy constructor while the other two forms do not.

That said, I can see your point about VariantMatcher VM = Arg.Value.getMatcher(); having the same issues when reading the code, so you're right that this isn't a particularly good example of why not to use auto.

I don't think you'll ever convince me that your way doesn't make the code worse :).

That's fine, I probably shouldn't have even tried to begin with. :-)

988

Ahh, thank you for the explanation, that makes more sense to me now.

clang/lib/ASTMatchers/Dynamic/Marshallers.h

 
 private:
   const unsigned MinCount;
   const unsigned MaxCount;
   const VarOp Op;
   const StringRef MatcherName;
 };
 
-template <typename CladeType, typename... MatcherT>
 class MapAnyOfMatcherDescriptor : public MatcherDescriptor {
-  std::vector<DynCastAllOfMatcherDescriptor> Funcs;
+  ASTNodeKind CladeNodeKind;
+  std::vector<ASTNodeKind> NodeKinds;
 
 public:
-  MapAnyOfMatcherDescriptor(StringRef MatcherName)
-      : Funcs{DynCastAllOfMatcherDescriptor(
-            ast_matchers::internal::VariadicDynCastAllOfMatcher<CladeType,
-                                                                MatcherT>{},
-            MatcherName)...} {}
+  MapAnyOfMatcherDescriptor(ASTNodeKind CladeNodeKind,
+                            std::vector<ASTNodeKind> NodeKinds)
+      : CladeNodeKind(CladeNodeKind), NodeKinds(NodeKinds) {}
 
   VariantMatcher create(SourceRange NameRange, ArrayRef<ParserValue> Args,
                         Diagnostics *Error) const override {
-    std::vector<VariantMatcher> InnerArgs;
 
-    for (auto const &F : Funcs) {
-      InnerArgs.push_back(F.create(NameRange, Args, Error));
-      if (!Error->errors().empty())
+    std::vector<DynTypedMatcher> NodeArgs;
+
+    for (auto NK : NodeKinds) {
+      std::vector<DynTypedMatcher> InnerArgs;
+
+      for (const auto &Arg : Args) {
+        if (!Arg.Value.isMatcher())
           return {};
+        const VariantMatcher &VM = Arg.Value.getMatcher();
+        if (VM.hasTypedMatcher(NK)) {
+          auto DM = VM.getTypedMatcher(NK);
+          InnerArgs.push_back(DM);
+        }
+      }
+
+      if (InnerArgs.empty()) {
+        NodeArgs.push_back(
+            DynTypedMatcher::trueMatcher(NK).dynCastTo(CladeNodeKind));
+      } else {
+        NodeArgs.push_back(
+            DynTypedMatcher::constructVariadic(
+                ast_matchers::internal::DynTypedMatcher::VO_AllOf, NK,
+                InnerArgs)
+                .dynCastTo(CladeNodeKind));
+      }
     }
-    return VariantMatcher::SingleMatcher(
-        ast_matchers::internal::BindableMatcher<CladeType>(
-            VariantMatcher::VariadicOperatorMatcher(
-                ast_matchers::internal::DynTypedMatcher::VO_AnyOf,
-                std::move(InnerArgs))
-                .getTypedMatcher<CladeType>()));
+
+    auto Result = DynTypedMatcher::constructVariadic(
+        ast_matchers::internal::DynTypedMatcher::VO_AnyOf, CladeNodeKind,
+        NodeArgs);
+    Result.setAllowBind(true);
+    return VariantMatcher::SingleMatcher(Result);
   }
 
   bool isVariadic() const override { return true; }
   unsigned getNumArgs() const override { return 0; }
 
   void getArgKinds(ASTNodeKind ThisKind, unsigned,
                    std::vector<ArgKind> &Kinds) const override {
     Kinds.push_back(ThisKind);
   }
 
   bool isConvertibleTo(ASTNodeKind Kind, unsigned *Specificity,
                        ASTNodeKind *LeastDerivedKind) const override {
-    return llvm::all_of(Funcs, [=](const auto &F) {
-      return F.isConvertibleTo(Kind, Specificity, LeastDerivedKind);
-    });
+    if (Specificity)
+      *Specificity = 1;
+    if (LeastDerivedKind)
+      *LeastDerivedKind = CladeNodeKind;
+    return true;
   }
 };
 
 /// Helper functions to select the appropriate marshaller functions.
 /// They detect the number of arguments, arguments types and return type.
 
 /// 0-arg overload
 template <typename ReturnType>
std::unique_ptr<MatcherDescriptor> makeMatcherAutoMarshall(
   return std::make_unique<VariadicOperatorMatcherDescriptor>(
       MinCount, MaxCount, Func.Op, MatcherName);
 }
 
 template <typename CladeType, typename... MatcherT>
 std::unique_ptr<MatcherDescriptor> makeMatcherAutoMarshall(
     ast_matchers::internal::MapAnyOfMatcherImpl<CladeType, MatcherT...>,
     StringRef MatcherName) {
-  return std::make_unique<MapAnyOfMatcherDescriptor<CladeType, MatcherT...>>(
-      MatcherName);
+  return std::make_unique<MapAnyOfMatcherDescriptor>(
+      ASTNodeKind::getFromNodeKind<CladeType>(),
+      std::vector<ASTNodeKind>{ASTNodeKind::getFromNodeKind<MatcherT>()...});
 }
 
 } // namespace internal
 } // namespace dynamic
 } // namespace ast_matchers
 } // namespace clang
 
 #endif // LLVM_CLANG_AST_MATCHERS_DYNAMIC_MARSHALLERS_H