Addressed review comments, with significant changes. Please rereview.
http://codereview.chromium.org/42115/diff/1/5 File src/objects-inl.h (right): http://codereview.chromium.org/42115/diff/1/5#newcode2321 Line 2321: case ATOM: return 0; On 2009/03/12 10:13:00, Erik Corry wrote: > Indentation is wrong here. Done. http://codereview.chromium.org/42115/diff/1/8 File src/regexp-macro-assembler-ia32.cc (right): http://codereview.chromium.org/42115/diff/1/8#newcode994 Line 994: String* subject_ptr = *subject; Rewritten to use handles everywhere. http://codereview.chromium.org/42115/diff/1/10 File src/runtime.cc (right): http://codereview.chromium.org/42115/diff/1/10#newcode1183 Line 1183: class ReplacementStringBuilder { On 2009/03/12 10:13:00, Erik Corry wrote: > I think you should use StringBuilderConcatHelper instead of Cons Strings. Done. http://codereview.chromium.org/42115/diff/1/10#newcode1215 Line 1215: ParseReplacement(replacement, capture_count); On 2009/03/12 10:13:00, Erik Corry wrote: > Nontrivial work shouldn't be in constructors. Done. http://codereview.chromium.org/42115/diff/1/10#newcode1238 Line 1238: FixedArray* match_info = last_match_info->elements(); As commented later, it can't be moved that far out. It could be moved out of this loop, but then it would be computed even when it's not used (e.g., replacing with a constant string). http://codereview.chromium.org/42115/diff/1/10#newcode1254 Line 1254: SUBJECT_MATCH = -1, True. Slightly faster to implement since I already have the match start and end indices computed. Probably not worth it. I'll remove it for now. http://codereview.chromium.org/42115/diff/1/10#newcode1258 Line 1258: REPLACEMENT_SUBSTRING = -5 On 2009/03/12 10:13:00, Erik Corry wrote: > I'd prefer these to be positive and the offsets negative. That way the switch > is with positive values which seems like the more common case for the C++ > compiler implementer. Done. http://codereview.chromium.org/42115/diff/1/10#newcode1267 Line 1267: int data; Comments added. http://codereview.chromium.org/42115/diff/1/10#newcode1297 Line 1297: parts->Add(ReplacementPart(last, i)); Parts is just a ZoneList, and this is a self-contained static function, so I don't think the overhead of moving those tests somewhere else is worth it. http://codereview.chromium.org/42115/diff/1/10#newcode1319 Line 1319: case '0': Yes and yes. http://codereview.chromium.org/42115/diff/1/10#newcode1356 Line 1356: default: With "$F00". http://codereview.chromium.org/42115/diff/1/10#newcode1435 Line 1435: FixedArray* match_data = last_match_info_handle->elements(); 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. To avoid creating a handle for every iteration, I just extract the pointer and use it immediately. http://codereview.chromium.org/42115/diff/1/10#newcode1504 Line 1504: CONVERT_CHECKED(JSArray, last_match_info, args[3]); I only ever use last_match_info after having executed a regular expression successfully. That should ensure the size as well as check that the array has fast elements. But yes, failing fast is a good thing, so I'll add an extra test here. http://codereview.chromium.org/42115/diff/1/12 File src/string.js (right): http://codereview.chromium.org/42115/diff/1/12#newcode239 Line 239: var reusableMatchInfo = [2, -1, -1, "", ""]; No, well spotted, the reusabeMatchArray isn't used any more. http://codereview.chromium.org/42115/diff/1/12#newcode254 Line 254: // the result. Sadly, yes. Replacing with a string pattern, instead of a RegExp, is still in Javascript (using indexOf), and it uses this for replacement. http://codereview.chromium.org/42115 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
