[PATCH] First revision of the dynamic ASTMatcher library

[Samuel Benzaquen]

  Fixed most of the comments on the parser/lexer. Some more are coming.

  wrt VariantValue:
   - The get<>() method asserts that the value is the right type.
   - Not using PointerUnion, because this will also contain bool and numbers later on.
   - The class itself should not leak any member, since on destruction/reassignment the content is destroyed. I could use OwningPtr<>s for the members, but then I could not use a union{} for all of them. I guess making this class larger is not much of a problem.


================
Comment at: include/clang/ASTMatchers/Dynamic/Diagnostics.h:91
@@ +90,3 @@
+  };
+  const llvm::ArrayRef<ErrorFrame> frames() const { return Frames; }
+
----------------
Dmitri Gribenko wrote:
> const llvm::ArrayRef -> llvm::ArrayRef, ArrayRef is intrinsically const.
Done.
It is a leftover from when I had const vector&.

================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:72
@@ +71,3 @@
+    processMatcherToken(const TokenInfo &MatcherInfo,
+                        llvm::ArrayRef<ParserValue> Args,
+                        Diagnostics *Error) = 0;
----------------
Dmitri Gribenko wrote:
> If you #include "clang/Basic/LLVM.h", you can drop 'llvm::' qualifications from StringRef and ArrayRef.
Done.

================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:42-43
@@ +41,4 @@
+
+/// \brief Matcher expression parser.
+class Parser {
+public:
----------------
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.

================
Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:36
@@ +35,3 @@
+  ///
+  /// /return The TokenInfo for the next token. It will continually return the
+  ///   empty token when the end of the input has been reached.
----------------
Dmitri Gribenko wrote:
> \return
Fixed.

================
Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:45
@@ +44,3 @@
+    if (Code.empty()) {
+      // Nothing to do.
+    } else if (isSingleCharToken(Code[0])) {
----------------
Dmitri Gribenko wrote:
> I don't think this is the best design (to produce an unlimited number of empty tokens at the end); all other lexers in LLVM and Clang usually produce an EOF token.
Fixed.
Added the enum, returns a TK_EOF kind at the end.

================
Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:57
@@ +56,3 @@
+      size_t TokenLength = 0;
+      while (TokenLength < Code.size() && !isspace(Code[TokenLength]) &&
+             !isSingleCharToken(Code[TokenLength]))
----------------
Dmitri Gribenko wrote:
> Please use functions from clang/Basic/CharInfo.h instead of ctype.
Fixed, a little. More to come.

================
Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:98
@@ +97,3 @@
+  void consumeWhitespace() {
+    while (!Code.empty() && isspace(Code[0])) {
+      if (Code[0] == '\n') {
----------------
Dmitri Gribenko wrote:
> Use clang/Basic/CharInfo.h
Done.

================
Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:145
@@ +144,3 @@
+  const TokenInfo &OpenToken = Tokenizer->getNextToken();
+  if (OpenToken.Token != "(") {
+    Error->pushErrorFrame(OpenToken, Error->ET_PARSER_NO_OPEN_PAREN);
----------------
Dmitri Gribenko wrote:
> String comparisons to compare tokens is not a good idea.
Fixed.

================
Comment at: lib/ASTMatchers/Dynamic/VariantValue.cpp:60-61
@@ +59,4 @@
+void VariantValue::setImpl(const std::string &NewValue) {
+  Type = VT_String;
+  Value.String = new std::string(NewValue);
+}
----------------
Dmitri Gribenko wrote:
> What if someone calls setImpl() repeatedly?  Do we just leak old values?
> 
I could call reset() from all the setImpl() overloads. Right now it is called once from the place where setImpl() is called.


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

BRANCH
  svn

ARCANIST PROJECT
  clang