[PATCH] First revision of the dynamic ASTMatcher library

[Dmitri Gribenko]

================
Comment at: lib/ASTMatchers/Dynamic/Diagnostics.cpp:34-35
@@ +33,4 @@
+  switch (Type) {
+    case Diagnostics::ET_REGISTRY_NOT_FOUND:
+      return "Matcher not found: $0";
+    case Diagnostics::ET_REGISTRY_WRONG_ARG_COUNT:
----------------
According to style guide, 'case' is not indented inside a switch().

================
Comment at: lib/ASTMatchers/Dynamic/Diagnostics.cpp:61-62
@@ +60,4 @@
+    case Diagnostics::ET_NONE:
+    default:
+      return "<N/A>";
+  }
----------------
'default' is discouraged in a fully-covered switch.  You'll need to add llvm_unreachable() after the switch to silence a warning from gcc.

================
Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:45
@@ +44,3 @@
+  TokenKind Kind;
+  SourceRange Location;
+};
----------------
IMHO, 'Range' would be a better name than 'Location'.

================
Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:88-89
@@ +87,4 @@
+
+    Result.Location.End.Line = Line;
+    Result.Location.End.Column = column() - 1;
+    return Result;
----------------
This makes 'End' inclusive.  In my opinion, this is not intuitive and not consistent with STL-like half-open ranges.

If you decide to make 'End' non-inclusive, you can also factor out a function to get a 'current' location (this piece of code is repeated twice in getNextToken()).

================
Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:93-94
@@ +92,4 @@
+
+  /// \brief Returns \c true when the whole input has been consumed.
+  bool isDone() const { return Code.empty(); }
+
----------------
This should not be needed after switching to an EOF token.

================
Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:67
@@ +66,3 @@
+      Result.Text = "";
+    } else if (isSingleCharToken(Code[0], &Result.Kind)) {
+      Result.Text = Code.substr(0, 1);
----------------
It would be better to inline the switch() from isSingleCharToken() here and turn the whole if-else chain into a switch().  This is idiomatic for lexers.

================
Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:270
@@ +269,3 @@
+    case TokenInfo::TK_COMMA:
+    default:
+      Error->pushErrorFrame(Token.Location, Error->ET_PARSER_INVALID_TOKEN)
----------------
'default' is discouraged in a fully-covered switch.


================
Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:242
@@ +241,3 @@
+/// parse function.
+bool parseToken(const TokenInfo &Token, CodeTokenizer *Tokenizer,
+                Parser::TokenProcessor *Processor, VariantValue *Value,
----------------
Why doesn't lexer do this?  Every token could have an associated value (optional, of course).


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

BRANCH
  svn

ARCANIST PROJECT
  clang