[PATCH] First revision of the dynamic ASTMatcher library

[Samuel Benzaquen]

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

================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:123
@@ +122,3 @@
+
+// static
+VariantValue Registry::constructMatcher(const std::string &MatcherName,
----------------
Manuel Klimek wrote:
> Comment left over?
Just commeting that this method is static.

================
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);
----------------
Manuel Klimek wrote:
> The style for loops in llvm is:
> for (size_t i = 0, e = Args.size(); i != e; ++i)
Done.

================
Comment at: lib/ASTMatchers/Dynamic/Marshallers.h:119
@@ +118,3 @@
+template <typename BaseType, typename DerivedType>
+class MatcherMarshallVariadic : public MatcherCreateCallback {
+public:
----------------
Manuel Klimek wrote:
> 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?
Done.

================
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)
----------------
Dmitri Gribenko wrote:
> Samuel Benzaquen wrote:
> > Dmitri Gribenko wrote:
> > > Manuel Klimek wrote:
> > > > understand*s*
> > > It would help a lot to give a formal definition of the grammar in documentation.
> > Done. I am a little rusty on how to specify grammars, so it might be off a little bit.
> This looks good for a start (I expect the grammar to become more complex as more features are added), but please wrap it into \code...\endcode so that it looks well in HTML pages.
Done.

================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:11-12
@@ +10,4 @@
+// Simple matcher expression parser.
+// The parser understand matcher expressions of the form:
+//   MatcherName(Arg0, Arg1, ..., ArgN)
+// as well as simple types like strings.
----------------
Samuel Benzaquen wrote:
> Dmitri Gribenko wrote:
> > Samuel Benzaquen wrote:
> > > Dmitri Gribenko wrote:
> > > > Manuel Klimek wrote:
> > > > > understand*s*
> > > > It would help a lot to give a formal definition of the grammar in documentation.
> > > Done. I am a little rusty on how to specify grammars, so it might be off a little bit.
> > This looks good for a start (I expect the grammar to become more complex as more features are added), but please wrap it into \code...\endcode so that it looks well in HTML pages.
> Done.
Yes. They will later include bool, numbers, etc.
The grammar itself should not be much more complex.

================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:41
@@ +40,3 @@
+
+  class TokenProcessor {
+  public:
----------------
Samuel Benzaquen wrote:
> Dmitri Gribenko wrote:
> > Manuel Klimek wrote:
> > > 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
> > > 
> > Documentation comment?
> Done.
Already added some comments. Let me know if you want something extra.

================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:54
@@ +53,3 @@
+
+  static VariantValue parseMatcher(llvm::StringRef MatcherCode,
+                                   TokenProcessor *Processor);
----------------
Manuel Klimek wrote:
> 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).
The result could also be an error. Having the error in the VariantValue makes many interfaces simpler.
In particular, it allows this method and all the factory methods to return errors.

Also, some of the test code actually returns strings here (the processor does it) for simplicity.

================
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,
----------------
Manuel Klimek wrote:
> Can we instead use Diagnostics for error reporting?
I don't know. It looks like Diagnostics works with SourceManager/SourceLocation.
I could make the error type have more diagnostic info, like line/col numbers.

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

================
Comment at: lib/ASTMatchers/Dynamic/Marshallers.h:43
@@ +42,3 @@
+
+// Generic callback implementation.
+// The |Marshaller| function will unpack the arguments, call |Func| and
----------------
Manuel Klimek wrote:
> 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)
Fixed using your other suggestion.

================
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
----------------
Manuel Klimek wrote:
> Please adapt comments in general to doxygen. Use \p Param for parameters, and \c Other for other code.
Done, I think.


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