================
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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
