LGTM, thanks for finding and fixing the issues! A couple of nits below.
http://codereview.chromium.org/43075/diff/1/16 File src/jsregexp.cc (right): http://codereview.chromium.org/43075/diff/1/16#newcode603 Line 603: matches = Factory::null_value(); I know this is not your code, but why is there an assignment to a local variable followed by a return? http://codereview.chromium.org/43075/diff/1/16#newcode740 Line 740: // Unimplemented on ARM, fall through to bytecode. Auch, this is hard to read. Can we factor this differently. http://codereview.chromium.org/43075/diff/1/10 File src/jsregexp.h (right): http://codereview.chromium.org/43075/diff/1/10#newcode118 Line 118: static int GetCapture(FixedArray* array, int index) { This is a general comment on this code. Don't let it stop you from putting back, but this code does not match the style of the rest of the V8 code. We usually put vertical space between functions to increase readability. It seems that there is very little vertical spacing in this file. http://codereview.chromium.org/43075/diff/1/7 File src/regexp-delay.js (right): http://codereview.chromium.org/43075/diff/1/7#newcode207 Line 207: throw MakeTypeError('method_called_on_incompatible', ['RegExp.prototype.test', this]); Long line, break it? http://codereview.chromium.org/43075/diff/1/14 File src/string.js (right): http://codereview.chromium.org/43075/diff/1/14#newcode302 Line 302: Any reason for the extra space here? http://codereview.chromium.org/43075 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
