Extract method to allow re-use

[steveire]

Summary

The list of matchers which can be used with a top matcher can be used in
other contexts than code completion. It can be output as data in
clang-tidy.

[aaron.ballman]

Generally LGTM but this has no test coverage. What are your plans for how you want to see this proceed? Do you intend to commit everything in one batch once all of the reviews are accepted, or do piecemeal commits?

lib/ASTMatchers/Dynamic/Registry.cpp

565

I'd prefer T to be named Callable and func to be Func. Also, I think it should be Callable &&Func.

595–637

Then you can use std::forward<Callable>(Func)(...); because we can't use std::invoke() yet.

[steveire]

I think this commit is fine without tests. The new method remains internal to the file but follow-up commits add public interface and tests.

I'll push the commits separately. I'll not be squashing them.

[steveire]

Update

[aaron.ballman]

In D54402#1295420, @steveire wrote:

I think this commit is fine without tests.

That's not something we do except under specific extenuating circumstances (NFC fixes or changes for which existing coverage suffices), per our developer policy.

The new method remains internal to the file but follow-up commits add public interface and tests.

I'll push the commits separately. I'll not be squashing them.

I'd prefer to squash them at least into commits that have tests for the functionality. More specifically: if the functionality is testable, it needs tests when it's committed. If it's not testable, squash it into the most closely-related commit that is testable. In terms of review cycles, I'm fine with "this functionality will be commit with that functionality, whose tests cover this.", though I'd personally prefer future reviews to come pre-squashed to cut down on confusion.

[steveire]

This is just a NFC change, which is normal to appear without tests. The consensus on IRC is that this is fine.

[aaron.ballman]

In D54402#1296273, @steveire wrote:

This is just a NFC change, which is normal to appear without tests. The consensus on IRC is that this is fine.

This NFC would likely be rejected were it not for your other patches because it would serve no purpose while adding complexity and overhead. I'm not certain why you feel so strongly about not merging this with other patches. We ask developers to do that for new features because it makes it trivial for us to know what to revert when a random bot goes red. That said, if you continue to feel strongly about this, I'll hold my nose.

LGTM with some formatting nits.

lib/ASTMatchers/Dynamic/Registry.cpp

565

80 col.

594–595

80 col and elide braces.

[steveire]

Update

lib/ASTMatchers/Dynamic/Registry.cpp

for (const ArgKind &Kind : TypeSet) {
         Ctor->getArgKinds(Kind.getMatcherKind(), ArgNumber, NextTypeSet);
     }
     TypeSet.clear();
     TypeSet.insert(NextTypeSet.begin(), NextTypeSet.end());
   }
   return std::vector<ArgKind>(TypeSet.begin(), TypeSet.end());
 }
 
-std::vector<MatcherCompletion>
-Registry::getMatcherCompletions(ArrayRef<ArgKind> AcceptedTypes) {
-  std::vector<MatcherCompletion> Completions;
+template <typename Callable>
+void processAcceptableMatchers(ArrayRef<ArgKind> AcceptedTypes, Callable &&Func) {
 
   // Search the registry for acceptable matchers.
   for (const auto &M : RegistryData->constructors()) {
     const MatcherDescriptor& Matcher = *M.getValue();
     StringRef Name = M.getKey();
 
     std::set<ASTNodeKind> RetKinds;
     unsigned NumArgs = Matcher.isVariadic() ? 1 : Matcher.getNumArgs();
for (const ArgKind& Kind : AcceptedTypes) {
         RetKinds.insert(LeastDerivedKind);
         for (unsigned Arg = 0; Arg != NumArgs; ++Arg)
           Matcher.getArgKinds(Kind.getMatcherKind(), Arg, ArgsKinds[Arg]);
         if (IsPolymorphic)
           break;
       }
     }
 
     if (!RetKinds.empty() && MaxSpecificity > 0) {
+      std::forward<Callable>(Func)(Name, Matcher, RetKinds, ArgsKinds, MaxSpecificity);
+    }
+  }
+}
+
+std::vector<MatcherCompletion>
+Registry::getMatcherCompletions(ArrayRef<ArgKind> AcceptedTypes) {
+  std::vector<MatcherCompletion> Completions;
+
+  processAcceptableMatchers(
+      AcceptedTypes,
+      [&Completions](StringRef Name, const MatcherDescriptor &Matcher,
+                     std::set<ASTNodeKind> &RetKinds,
+                     std::vector<std::vector<ArgKind>> ArgsKinds,
+                     unsigned MaxSpecificity) {
         std::string Decl;
         llvm::raw_string_ostream OS(Decl);
 
-      if (IsPolymorphic) {
+        if (Matcher.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;
             // Two steps. First all non-matchers, then matchers only.
             for (const ArgKind &AK : Arg) {
               if (AK.getArgKind() == ArgKind::AK_Matcher) {
                 MatcherKinds.insert(AK.getMatcherKind());
               } else {
-              if (!FirstArgKind) OS << "|";
+                if (!FirstArgKind)
+                  OS << "|";
                 FirstArgKind = false;
                 OS << AK.asString();
               }
             }
             if (!MatcherKinds.empty()) {
-            if (!FirstArgKind) OS << "|";
+              if (!FirstArgKind)
+                OS << "|";
               OS << "Matcher<" << MatcherKinds << ">";
             }
           }
         }
         if (Matcher.isVariadic())
           OS << "...";
         OS << ")";
 
         std::string TypedText = Name;
         TypedText += "(";
         if (ArgsKinds.empty())
           TypedText += ")";
         else if (ArgsKinds[0][0].getArgKind() == ArgKind::AK_String)
           TypedText += "\"";
 
         Completions.emplace_back(TypedText, OS.str(), MaxSpecificity);
-    }
-  }
+      });
 
   return Completions;
 }
 
 VariantMatcher Registry::constructMatcher(MatcherCtor Ctor,
                                           SourceRange NameRange,
                                           ArrayRef<ParserValue> Args,
                                           Diagnostics *Error) {