LGTM
http://codereview.chromium.org/14190/diff/208/8 File src/ast.h (right): http://codereview.chromium.org/14190/diff/208/8#newcode1222 Line 1222: virtual int min_match() = 0; Could we call this min_chars instead? Seems less ambiguous to me. http://codereview.chromium.org/14190/diff/208/8#newcode1238 Line 1238: min_match_(alternatives->at(0)->min_match()), I think these initializations should be moved into the body of the constructor, since they will be overwritten later. http://codereview.chromium.org/14190/diff/208/8#newcode1241 Line 1241: RegExpTree* alternative = alternatives->at(i); We have Min and Max in utils.h for stuff like this. Perhaps with the help of those we can get this down to a size where it is not unreasonable to have it in the .h file. http://codereview.chromium.org/14190/diff/208/8#newcode1246 Line 1246: if (max_match_ != kInfinity) { You don't need to special case infinity here. http://codereview.chromium.org/14190/diff/208/8#newcode1299 Line 1299: Reordering stuff like this makes the change hard to read. http://codereview.chromium.org/14190/diff/208/8#newcode1382 Line 1382: void AddElement(TextElement elm) { I think TextElement should grow a Length() method. http://codereview.chromium.org/14190/diff/208/8#newcode1392 Line 1392: UNIMPLEMENTED(); unreachable http://codereview.chromium.org/14190/diff/208/8#newcode1409 Line 1409: min_match_(min * body->min_match()), move into body http://codereview.chromium.org/14190/diff/208/8#newcode1411 Line 1411: if (max > 0 && body->max_match() > kInfinity / max) { Please add a test with a max of 0. http://codereview.chromium.org/14190 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
