[PATCH] First revision of the dynamic ASTMatcher library

[Manuel Klimek]

================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:11
@@ +10,3 @@
+// Simple matcher expression parser.
+// The parser understand matcher expressions of the form:
+//   MatcherName(Arg0, Arg1, ..., ArgN)
----------------
understand*s*

================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:41
@@ +40,3 @@
+
+  class TokenProcessor {
+  public:
----------------
Since this is an interface for the Parser, I think this requires a lot more documentation to explain
a) why it is an interface (instead of just built-in)
b) why processValueToken has a default implementation


================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:54
@@ +53,3 @@
+
+  static VariantValue parseMatcher(llvm::StringRef MatcherCode,
+                                   TokenProcessor *Processor);
----------------
I'm wondering whether we want to call the return value differently to make sure it's usually a matcher? (I assume it's called value because it can also be just a literal value type).

================
Comment at: include/clang/ASTMatchers/Dynamic/Registry.h:31-32
@@ +30,4 @@
+  // matcher by name.
+  // It will return VariantError if the matcher is not found, or if the
+  // number of arguments or argument types do not match the signature.
+  static VariantValue constructMatcher(const std::string &MatcherName,
----------------
Can we instead use Diagnostics for error reporting?

================
Comment at: include/clang/ASTMatchers/Dynamic/VariantValue.h:48
@@ +47,3 @@
+
+class VariantValue {
+ public:
----------------
This needs some doc on how this is supposed to be used.

================
Comment at: lib/ASTMatchers/Dynamic/Marshallers.h:44
@@ +43,3 @@
+// Generic callback implementation.
+// The |Marshaller| function will unpack the arguments, call |Func| and
+// repack the return value. It should also check that the arguments are
----------------
Please adapt comments in general to doxygen. Use \p Param for parameters, and \c Other for other code.

================
Comment at: lib/ASTMatchers/Dynamic/Marshallers.h:43
@@ +42,3 @@
+
+// Generic callback implementation.
+// The |Marshaller| function will unpack the arguments, call |Func| and
----------------
Instead of calling it MatcherCreateCallbackImpl, perhaps call it GenericMatcherCreateCallback, which would basically explain what it does immediately :)

(I usually dislike *Impl postfixes, especially if they're not the only implementation)

================
Comment at: lib/ASTMatchers/Dynamic/Marshallers.h:129
@@ +128,3 @@
+    std::vector<const DerivedMatcherType *> InnerArgs(Args.size());
+    for (size_t i = 0; i < Args.size(); ++i) {
+      CHECK_ARG_TYPE(i, DerivedMatcherType);
----------------
The style for loops in llvm is:
for (size_t i = 0, e = Args.size(); i != e; ++i)

================
Comment at: lib/ASTMatchers/Dynamic/Marshallers.h:119
@@ +118,3 @@
+template <typename BaseType, typename DerivedType>
+class MatcherMarshallVariadic : public MatcherCreateCallback {
+public:
----------------
Perhaps call it VariadicMatcherCreateCallback, so it's clear what it is from the name. Can the other than even be called FixedArgCountMatcherCreateCallback or something similar?

================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:123
@@ +122,3 @@
+
+// static
+VariantValue Registry::constructMatcher(const std::string &MatcherName,
----------------
Comment left over?

================
Comment at: unittests/ASTMatchers/Dynamic/RegistryTest.cpp:1
@@ +1,2 @@
+//===- unittest/ASTMatchers/Dynamic/RegistryTest.cpp - Registry unit tests
+//-===//
----------------
Unintentional break?


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