LGTM
http://codereview.chromium.org/17378/diff/1/4 File src/jsregexp.cc (right): http://codereview.chromium.org/17378/diff/1/4#newcode1378 Line 1378: assembler->PushRegister(reg, check_stack_limit); It's a waste to push... http://codereview.chromium.org/17378/diff/1/4#newcode1389 Line 1389: if (affected_registers.Get(reg)) assembler->PopRegister(reg); ... and pop registers if they are just affected by a WritePosition. We could just clear them instead of popping them. http://codereview.chromium.org/17378/diff/1/4#newcode1428 Line 1428: case ActionNode::STORE_POSITION: { We should be ignoring a store position that we find after we found a clear... http://codereview.chromium.org/17378/diff/1/4#newcode1438 Line 1438: case ActionNode::CLEAR_CAPTURES: { ...and we should be ignoring a clear that we find after a store position. http://codereview.chromium.org/17378/diff/1/4#newcode3661 Line 3661: } else if (!needs_capture_clearing) { Surely it's OK to unroll and no need to clear captures if max is 1? http://codereview.chromium.org/17378/diff/1/5 File src/jsregexp.h (right): http://codereview.chromium.org/17378/diff/1/5#newcode717 Line 717: RegExpNode* on_success); This fits on one line. http://codereview.chromium.org/17378/diff/1/5#newcode764 Line 764: int range_from; Can't we just have an Interval here? http://codereview.chromium.org/17378/diff/1/5#newcode1011 Line 1011: bool Mentions(int reg); It's messy that this is not a private method, since it returns an invalid register for some actions. How about a public method that returns a (perhaps trivial) Interval instead? http://codereview.chromium.org/17378 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
