Add clang-query support for mapAnyOf

[aaron.ballman]

LGTM aside from some minor nits.

clang/lib/ASTMatchers/Dynamic/Marshallers.h

1081

Is this a TODO comment?

1095–1098

1099–1100

[steveire]

Update

[steveire]

@aaron.ballman Please review this again as I've merged some more needed changes into it.

[aaron.ballman]

clang/include/clang/ASTMatchers/Dynamic/Registry.h

36–37 ↗

(On Diff #319431)

40 ↗

(On Diff #319431)

I think it'd be cleaner to make callers be explicit about forming the smart pointer.

clang/lib/ASTMatchers/Dynamic/Marshallers.h

586–587

Might as well make it obvious the function doesn't care about its arguments (same elsewhere in the patch).

1080–1082

clang/lib/ASTMatchers/Dynamic/Parser.cpp

372 ↗

(On Diff #319431)

373 ↗

(On Diff #319431)

435 ↗

(On Diff #319431)

449 ↗

(On Diff #319431)

The comment is a bit misleading in that this doesn't parse the .bind part, only the ("foo") bits.

Also, why does the function now accept BindToken but not use it?

491 ↗

(On Diff #319431)

510 ↗

(On Diff #319431)

521 ↗

(On Diff #319431)

553 ↗

(On Diff #319431)

unique_ptr? or is this our custom smart pointer? Either way, please spell the type out.

563 ↗

(On Diff #319431)

638–641 ↗

(On Diff #319431)

691 ↗

(On Diff #319431)

clang/lib/ASTMatchers/Dynamic/Registry.cpp

580

I think that having the explicit ctor call here would make it more obvious that the .release() is being picked up by another smart pointer type.

654

Any reason this declaration needed to move?

[steveire]

Update

clang/lib/ASTMatchers/Dynamic/Registry.cpp

654

It's used after the if-else.

[aaron.ballman]

I think there should be some documentation change for the new with functionality.

clang/lib/ASTMatchers/Dynamic/Marshallers.h

397–406

Rather than repeat these degenerate definitions six times, should this just be the default implementation within MatcherDescriptor (so derived classes are not required to override them)? Alternatively, should we make a NonBuilderMatcherDescriptor class that holds the degenerate definition, and these derived classes can inherit from NonBuilderMatcherDescriptor instead?

1093

clang/lib/ASTMatchers/Dynamic/Parser.cpp

373 ↗

(On Diff #320593)

449 ↗

(On Diff #320593)

606 ↗

(On Diff #320593)

[steveire]

Update

[steveire]

In D94880#2536399, @aaron.ballman wrote:

I think there should be some documentation change for the new with functionality.

It's already documented in https://clang.llvm.org/docs/LibASTMatchersReference.html , just like all the other functionality.

[steveire]

@aaron.ballman Any further comments?

[aaron.ballman]

In D94880#2544782, @steveire wrote:

@aaron.ballman Any further comments?

Thank you for pinging, this fell off my queue! The only further comments I have about about my confusion with with.

In D94880#2536485, @steveire wrote:

In D94880#2536399, @aaron.ballman wrote:

I think there should be some documentation change for the new with functionality.

It's already documented in https://clang.llvm.org/docs/LibASTMatchersReference.html , just like all the other functionality.

Perhaps I'm blind, but when I search for "with" on that page, I get 21 hits and none of them are about the with() function. I am not finding evidence of with being supported on ToT, can you point me to the existing implementation? (If the functionality isn't being introduced in this patch, you don't need to document as part of this work. However, I was under the impression you were adding this functionality, so I'd like to understand this situation better before approving.)

[steveire]

That page collapses the documentation for each matcher, as you know.

You need to expand the docs for mapAnyOf, as you know.

You seem to know all this. What's the remaining issue?

[aaron.ballman]

In D94880#2544873, @steveire wrote:

That page collapses the documentation for each matcher, as you know.

You need to expand the docs for mapAnyOf, as you know.

You seem to know all this. What's the remaining issue?

The issue was that this was not at all obvious to me as a code reviewer, so I was asking you for help. Your response comes across as patronizing, but thank you for giving me the information I was looking for. FWIW, I did not know this was under the mapAnyOf matcher and I was assuming this was going to be documented at the top level, just like bind().

LG.

[steveire]

In D94880#2544895, @aaron.ballman wrote:

In D94880#2544873, @steveire wrote:

That page collapses the documentation for each matcher, as you know.

You need to expand the docs for mapAnyOf, as you know.

You seem to know all this. What's the remaining issue?

The issue was that this was not at all obvious to me as a code reviewer, so I was asking you for help. Your response comes across as patronizing, but thank you for giving me the information I was looking for. FWIW, I did not know this was under the mapAnyOf matcher and I was assuming this was going to be documented at the top level, just like bind().

LG.

You reviewed the patches for mapAnyOf up to now and you reviewed this change which has unit tests using with. I didn't know what was unclear to you. Sorry for being terse.

Thanks for the review.

clang/lib/ASTMatchers/Dynamic/Marshallers.h

FixedArgCountMatcherDescriptor(MarshallerType Marshaller, void (*Func)(),
         ArgKinds(ArgKinds.begin(), ArgKinds.end()) {}
 
   VariantMatcher create(SourceRange NameRange,
                         ArrayRef<ParserValue> Args,
                         Diagnostics *Error) const override {
     return Marshaller(Func, MatcherName, NameRange, Args, Error);
   }
 
   bool isBuilderMatcher() const override { return false; }
 
   std::unique_ptr<MatcherDescriptor>
   buildMatcherCtor(SourceRange NameRange, ArrayRef<ParserValue> Args,
                    Diagnostics *Error) const override {
     return {};
   }
 
   ASTNodeKind nodeMatcherType() const override { return ASTNodeKind(); }
 
   bool isVariadic() const override { return false; }
   unsigned getNumArgs() const override { return ArgKinds.size(); }
 
   void getArgKinds(ASTNodeKind ThisKind, unsigned ArgNo,
                    std::vector<ArgKind> &Kinds) const override {
     Kinds.push_back(ArgKinds[ArgNo]);
   }
 
VariantMatcher create(SourceRange NameRange,
                         ArrayRef<ParserValue> Args,
                         Diagnostics *Error) const override {
     return Func(MatcherName, NameRange, Args, Error);
   }
 
   bool isBuilderMatcher() const override { return false; }
 
   std::unique_ptr<MatcherDescriptor>
   buildMatcherCtor(SourceRange NameRange, ArrayRef<ParserValue> Args,
                    Diagnostics *Error) const override {
     return {};
   }
 
   ASTNodeKind nodeMatcherType() const override { return ArgsNodeKind; }
 
   bool isVariadic() const override { return true; }
   unsigned getNumArgs() const override { return 0; }
 
public:
   buildMatcherCtor(SourceRange NameRange, ArrayRef<ParserValue> Args,
                    Diagnostics *Error) const override {
     return {};
   }
 
   ASTNodeKind nodeMatcherType() const override { return ASTNodeKind(); }
 };
 
+class MapAnyOfBuilderDescriptor : public MatcherDescriptor {
+public:
+  VariantMatcher create(SourceRange NameRange, ArrayRef<ParserValue> Args,
+                        Diagnostics *Error) const override {
+    return {};
+  }
+
+  bool isBuilderMatcher() const override { return true; }
+
+  std::unique_ptr<MatcherDescriptor>
+  buildMatcherCtor(SourceRange NameRange, ArrayRef<ParserValue> Args,
+                   Diagnostics *Error) const override {
+
+    std::vector<ASTNodeKind> NodeKinds;
+    for (auto Arg : Args) {
+      if (!Arg.Value.isNodeKind())
+        return {};
+      NodeKinds.push_back(Arg.Value.getNodeKind());
+    }
+
+    if (NodeKinds.empty()) {
+      // Set error
+      return {};
+    }
+
+    ASTNodeKind CladeNodeKind = NodeKinds.front().getCladeKind();
+
+    return std::make_unique<MapAnyOfMatcherDescriptor>(CladeNodeKind,
+                                                       NodeKinds);
+  }
+
+  bool isVariadic() const override { return false; }
+
+  unsigned getNumArgs() const override { return 0; }
+
+  void getArgKinds(ASTNodeKind ThisKind, unsigned ArgNo,
+                   std::vector<ArgKind> &ArgKinds) const override {
+    return;
+  }
+  bool isConvertibleTo(ASTNodeKind Kind, unsigned *Specificity = nullptr,
+                       ASTNodeKind *LeastDerivedKind = nullptr) const override {
+    return false;
+  }
+
+  bool isPolymorphic() const override { return false; }
+
+  ASTNodeKind nodeMatcherType() const override { return ASTNodeKind(); }
+};
+
 /// 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(ReturnType (*Func)(), StringRef MatcherName) {
   std::vector<ASTNodeKind> RetTypes;

clang/lib/ASTMatchers/Dynamic/Registry.cpp

RegistryMaps::RegistryMaps() {
   // TODO: Here is the list of the missing matchers, grouped by reason.
   //
   // Polymorphic + argument overload:
   // findAll
   //
   // Other:
   // equalsNode
 
+  registerMatcher("mapAnyOf",
+                  std::make_unique<internal::MapAnyOfBuilderDescriptor>());
+
   REGISTER_OVERLOADED_2(callee);
   REGISTER_OVERLOADED_2(hasAnyCapture);
   REGISTER_OVERLOADED_2(hasPrefix);
   REGISTER_OVERLOADED_2(hasType);
   REGISTER_OVERLOADED_2(ignoringParens);
   REGISTER_OVERLOADED_2(isDerivedFrom);
   REGISTER_OVERLOADED_2(isDirectlyDerivedFrom);
   REGISTER_OVERLOADED_2(isSameOrDerivedFrom);

 static llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
                                      const std::set<ASTNodeKind> &KS) {
   unsigned Count = 0;
   for (std::set<ASTNodeKind>::const_iterator I = KS.begin(), E = KS.end();
        I != E; ++I) {
     if (I != KS.begin())
       OS << "|";
     if (Count++ == 3) {
       OS << "...";
       break;
     }
     OS << *I;
   }
   return OS;
 }
 
 std::vector<ArgKind> Registry::getAcceptedCompletionTypes(
for (const ArgKind& Kind : AcceptedTypes) {
       }
     }
 
     if (!RetKinds.empty() && MaxSpecificity > 0) {
       std::string Decl;
       llvm::raw_string_ostream OS(Decl);
 
       if (IsPolymorphic) {
         OS << "Matcher<T> " << Name << "(Matcher<T>";
       } else {
         OS << "Matcher<" << RetKinds << "> " << Name << "(";
         for (const std::vector<ArgKind> &Arg : ArgsKinds) {
           if (&Arg != &ArgsKinds[0])
             OS << ", ";
 
           bool FirstArgKind = true;
           std::set<ASTNodeKind> MatcherKinds;