LGTM if it lints, passes tests and compiles on ARM.
http://codereview.chromium.org/42115/diff/1/10 File src/runtime.cc (right): http://codereview.chromium.org/42115/diff/1/10#newcode1435 Line 1435: FixedArray* match_data = last_match_info_handle->elements(); On 2009/03/13 08:36:51, Lasse Reichstein wrote: > Won't work. The loop might be preempted (since it calls RegExps) and some other > code might extend the last_match_info array, so it's not safe to retain a > reference to the current backing FixedArray. We need to extract it on every > iteration. If you retain a reference to the old FixedArray then it will not be collected and you can write your captures to it. The static properties on RegExp will not reflect your changes but if you are using regexps in multiple threads that is not unexpected. But we can save that for another afsnit. > To avoid creating a handle for every iteration, I just extract the pointer and > use it immediately. http://codereview.chromium.org/42115/diff/1/12 File src/string.js (right): http://codereview.chromium.org/42115/diff/1/12#newcode254 Line 254: // the result. On 2009/03/13 08:36:51, Lasse Reichstein wrote: > Sadly, yes. Replacing with a string pattern, instead of a RegExp, is still in > Javascript (using indexOf), and it uses this for replacement. But knowing that number of captures is always zero should permit simplifications here. http://codereview.chromium.org/42115/diff/17/26 File src/runtime.cc (right): http://codereview.chromium.org/42115/diff/17/26#newcode1207 Line 1207: if (length < (1 << 11) && from < (1 << 19)) { Not your fault (it's mine :-) but 11 and 19 should be named constants. http://codereview.chromium.org/42115/diff/17/26#newcode1211 Line 1211: Handle<String> slice = Factory::NewStringSlice(subject_, from, to); Do we have test coverage of this branch? http://codereview.chromium.org/42115/diff/17/26#newcode1309 Line 1309: void Compile(Handle<String> replacement, This method is (way!) too big to be inline in the class declaration. http://codereview.chromium.org/42115/diff/17/26#newcode1351 Line 1351: void Apply(ReplacementStringBuilder* builder, Ditto. http://codereview.chromium.org/42115/diff/17/26#newcode1590 Line 1590: // Guessing the number of parts that the final result string is build built http://codereview.chromium.org/42115 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
