LGTM, with comments. Primarily looked at parser.cc
http://codereview.chromium.org/10759/diff/14/206 File src/parser.cc (right): http://codereview.chromium.org/10759/diff/14/206#newcode260 Line 260: last_ = list_->RemoveLast(); This allows removing more than the last added element from the buffer. Removing the last element is only a feature to support adding parsed quantifiers after adding the atom it quantifies. The method (or class for that matter) wasn't intended to be general purpose. Is there a current need for this feature? http://codereview.chromium.org/10759/diff/14/206#newcode315 Line 315: void AddTerm(RegExpTree* tree); I would prefer if this is still called AddAtom (see below). http://codereview.chromium.org/10759/diff/14/206#newcode368 Line 368: terms_.Add(text); The other places where a buffer is used, the contents are extracted into a single list, using GetList(). Is there an advantage to building a RegExpText incrementally instead of passing it a single ZoneList (which already exists)? http://codereview.chromium.org/10759/diff/14/206#newcode389 Line 389: void RegExpBuilder::AddTerm(RegExpTree* term) { The original method was called AddAtom to match the grammar productions. An Atom is something that can be quantified. A Term cannot have a quantifier applied. I would keep this method called AddAtom, and make another called AddText (to match AddCharacter) or AddCharacterClass (because that's the only text content that is added directly and not using AddCharacter). http://codereview.chromium.org/10759/diff/14/206#newcode467 Line 467: ASSERT(last_added_ == ADD_ATOM); The last thing added was a character class. The last_added property is pure debugging, so it should be as specific as possible. I would add an ADD_CHAR_CLASS to represent it. http://codereview.chromium.org/10759 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
