[PATCH] First revision of the dynamic ASTMatcher library

[Samuel Benzaquen]

  The way it works is like this:
  In Marshallers.h I use template argument deduction to determine the signature of the function I am wrapping. Using the deduced arguments, the marshaller function calls is<ArgType>() and get<ArgType>() to verify the arguments and call the wrapped function.
  Somewhere there needs to be some dispatch magic that can take the deduced types and verify/get the right stuff from VariantValue. I put this magic in VariantValue itself.
  The other thing I could do is to have simple methods in VariantValue like isString()/getString()/isDynMatcher()/getDynMatcher(), etc. and do the overload magic inside of Marshallers.h.
  I am ok either way.
  Please consider that I will be adding bool and numbers to VariantValue, so making the change now rather than later would be best.


================
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:
----------------
Dmitri Gribenko wrote:
> According to style guide, 'case' is not indented inside a switch().
Fixed.
I am fighting vi here... it is configured for another style.

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

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

================
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);
----------------
Dmitri Gribenko wrote:
> 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.
Done.

================
Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:88-89
@@ +87,4 @@
+
+    Result.Location.End.Line = Line;
+    Result.Location.End.Column = column() - 1;
+    return Result;
----------------
Manuel Klimek wrote:
> Dmitri Gribenko wrote:
> > 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()).
> IIRC source ranges have an inclusive end in clang (they usually point at the beginning of the last token of a range).
Done.

================
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(); }
+
----------------
Dmitri Gribenko wrote:
> This should not be needed after switching to an EOF token.
Removed.

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

================
Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:242
@@ +241,3 @@
+/// parse function.
+bool parseToken(const TokenInfo &Token, CodeTokenizer *Tokenizer,
+                Parser::TokenProcessor *Processor, VariantValue *Value,
----------------
Dmitri Gribenko wrote:
> Why doesn't lexer do this?  Every token could have an associated value (optional, of course).
It could do the rest of the parsing, except for the matchers. I would have to pass more state to it, though.

================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:42-43
@@ +41,4 @@
+
+/// \brief Matcher expression parser.
+class Parser {
+public:
----------------
Dmitri Gribenko wrote:
> Samuel Benzaquen wrote:
> > Dmitri Gribenko wrote:
> > > This class just holds a few static member functions.  Is the class really needed here?
> > It is not really needed. I could take out the class / rename as namespace.
> > The same applies to Registry, I guess.
> Or you could take advantage of 'Parser' being a real class: if it is appropriate, you could move 'TokenProcessor' and 'Diagnostics' to be class members.
Done.


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

BRANCH
  svn

ARCANIST PROJECT
  clang