[PATCH] First revision of the dynamic ASTMatcher library

[Dmitri Gribenko]

================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:35-38
@@ +34,6 @@
+    llvm::StringRef Token;
+    int StartLine;
+    int EndLine;
+    int StartColumn;
+    int EndColumn;
+  };
----------------
'unsigned'?

================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:58
@@ +57,3 @@
+private:
+  Parser(); // Declared, but not defined.
+};
----------------
Use LLVM_DELETED_FUNCTION.

================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:50
@@ +49,3 @@
+    processMatcherToken(llvm::StringRef MatcherName,
+                        const std::vector<VariantValue> &Args,
+                        const TokenInfo &Info) = 0;
----------------
Why not ArrayRef?

================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:43
@@ +42,3 @@
+  public:
+    virtual ~TokenProcessor() {}
+    virtual VariantValue processValueToken(const VariantValue &Value,
----------------
This should be defined in .cpp file, probably (or vtable will be emitted into every object file).

================
Comment at: include/clang/ASTMatchers/Dynamic/Registry.h:33-34
@@ +32,4 @@
+  // number of arguments or argument types do not match the signature.
+  static VariantValue constructMatcher(const std::string &MatcherName,
+                                       const std::vector<VariantValue> &Args);
+
----------------
StringRef, ArrayRef?

================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:41
@@ +40,3 @@
+
+  class TokenProcessor {
+  public:
----------------
Documentation comment?

================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:10-16
@@ +9,9 @@
+//
+// Simple matcher expression parser.
+// The parser understand matcher expressions of the form:
+//   MatcherName(Arg0, Arg1, ..., ArgN)
+// as well as simple types like strings.
+// The parser does not know how to process the matchers. It delegates this task
+// to a TokenProcessor object received as an argument.
+//
+//===----------------------------------------------------------------------===//
----------------
Three slashes, '\file'.

================
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.
----------------
It would help a lot to give a formal definition of the grammar in documentation.

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:308
@@ +307,3 @@
+  /// \brief Makes a copy of this matcher object.
+  virtual Matcher<T>* clone() const {
+    return new Matcher<T>(*this);
----------------
Star goes to the right.

================
Comment at: include/clang/ASTMatchers/Dynamic/Registry.h:37
@@ +36,3 @@
+private:
+  Registry(); // Declared, but not defined.
+};
----------------
Use LLVM_DELETED_FUNCTION.

================
Comment at: include/clang/ASTMatchers/Dynamic/Registry.h:33
@@ +32,3 @@
+  // number of arguments or argument types do not match the signature.
+  static VariantValue constructMatcher(const std::string &MatcherName,
+                                       const std::vector<VariantValue> &Args);
----------------
Dmitri Gribenko wrote:
> StringRef, ArrayRef?
Three slashes.

================
Comment at: include/clang/ASTMatchers/Dynamic/VariantValue.h:40
@@ +39,3 @@
+struct VariantValueReturnType {
+  typedef const T& type;
+};
----------------
& goes to the right.

================
Comment at: include/clang/ASTMatchers/Dynamic/VariantValue.h:35-37
@@ +34,5 @@
+
+// Helper template metafunction to determine the return type of
+// VariantValue::get(). For Matcher<T> we return by value, since we do matcher
+// conversion on demand.
+template <class T>
----------------
Three slashes.

================
Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:105-106
@@ +104,4 @@
+  llvm::StringRef StartOfLine;
+  int Line;
+  int column() const { return Code.data() - StartOfLine.data() + 1; }
+};
----------------
unsigned?


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