[PATCH] Introduce Registry::getCompletions.

[Peter Collingbourne]

================
Comment at: include/clang/AST/ASTTypeTraits.h:67
@@ -66,1 +66,3 @@
 
+  bool operator<(const ASTNodeKind &Other) const {
+    return KindId < Other.KindId;
----------------
Samuel Benzaquen wrote:
> Comment what is the meaning of A<B. It can't be used as a meaningful comparison between the types.
Done.

================
Comment at: include/clang/ASTMatchers/Dynamic/Registry.h:38
@@ +37,3 @@
+  MatcherCompletion() {}
+  MatcherCompletion(std::string TypedText, std::string MatcherDecl)
+      : TypedText(TypedText), MatcherDecl(MatcherDecl) {}
----------------
Samuel Benzaquen wrote:
> StringRef
Done.

================
Comment at: lib/ASTMatchers/Dynamic/Marshallers.h:207
@@ +206,3 @@
+        if (Specificity)
+          *Specificity = 100 - Distance;
+        if (LeastDerivedKind)
----------------
Samuel Benzaquen wrote:
> 100?
Arbitrary.  I think that if we have an inheritance hierarchy 100 levels deep we probably have bigger problems than this constant :)

================
Comment at: lib/ASTMatchers/Dynamic/Marshallers.h:363
@@ +362,3 @@
+                       ast_type_traits::ASTNodeKind *LeastDerivedKind) const {
+    for (std::vector<ast_type_traits::ASTNodeKind>::const_iterator
+             i = RetKinds.begin(),
----------------
Samuel Benzaquen wrote:
> This is repeated with FixedArgCountMatcherDesc. Factor this out into a function.
Done.

================
Comment at: lib/ASTMatchers/Dynamic/Marshallers.h:382
@@ -147,1 +381,3 @@
   const std::string MatcherName;
+  std::vector<ast_type_traits::ASTNodeKind> RetKinds;
+  const ArgKind ArgsKind;
----------------
Samuel Benzaquen wrote:
> You could make this field also const if BuildReturnTypeVector<> returned the vector instead of taking by ref.
Probably, but I don't think it matters too much whether this is const or not. All the member functions are const qualified anyway.

================
Comment at: lib/ASTMatchers/Dynamic/Marshallers.h:549
@@ +548,3 @@
+      if ((*I)->isConvertibleTo(ThisKind))
+        (*I)->getArgKinds(ThisKind, ArgNo, Kinds);
+    }
----------------
Samuel Benzaquen wrote:
> This has the potential to give invalid autocomplete.
> For example, the first argument only matches Overloads[0], but the second argument autocompletes with Overloads[1] ArgKind.
> At least add a note about it. We might want to fix it somehow later on.
Done. (This also applies to other matcher descriptors so I added the note to the base class.) This will probably have to be done by passing more context information through to getArgKinds.

================
Comment at: lib/ASTMatchers/Dynamic/Marshallers.h:541
@@ -339,2 +540,3 @@
 
- private:
+  bool isVariadic() const { return Overloads[0]->isVariadic(); }
+  unsigned getNumArgs() const { return Overloads[0]->getNumArgs(); }
----------------
Samuel Benzaquen wrote:
> Please add a check that all the overloads have the same properties.
Done.

================
Comment at: lib/ASTMatchers/Dynamic/Marshallers.h:605
@@ +604,3 @@
+  }
+  bool isPolymorphic() const LLVM_OVERRIDE { return true; }
+
----------------
Samuel Benzaquen wrote:
> You are not using LLVM_OVERRIDE consistently.
> You are missing it in many of the overrides.
I'm using it consistently wherever I intend to override a non-pure virtual function.

================
Comment at: lib/ASTMatchers/Dynamic/Marshallers.h:663
@@ +662,3 @@
+/// Not strictly necessary, but DynCastAllOfMatcherDesc gives us better
+/// completion results for that type of matcher.
+template <typename BaseT, typename DerivedT>
----------------
Samuel Benzaquen wrote:
> Is the overload above used if you add this one?
No. We check on lines 409 and 410 of RegistryTest.cpp that the correct overload is chosen for dyncast matchers.

================
Comment at: lib/ASTMatchers/Dynamic/Marshallers.h:258
@@ +257,3 @@
+
+template <typename T>
+void buildReturnTypeVectorFromTypeList(
----------------
Samuel Benzaquen wrote:
> Comment these.
I think the function names are self explanatory.


http://llvm-reviews.chandlerc.com/D2210