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; Indentation is wrong here. 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; Please add an AssertNoAllocation object here. 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 { I think you should use StringBuilderConcatHelper instead of Cons Strings. http://codereview.chromium.org/42115/diff/1/10#newcode1215 Line 1215: ParseReplacement(replacement, capture_count); Nontrivial work shouldn't be in constructors. http://codereview.chromium.org/42115/diff/1/10#newcode1238 Line 1238: FixedArray* match_info = last_match_info->elements(); Move outside loop? Actually, you should move this outside the function completely so you can get it outside the /g loop. http://codereview.chromium.org/42115/diff/1/10#newcode1254 Line 1254: SUBJECT_MATCH = -1, Subject match seems like a special case of subject capture. http://codereview.chromium.org/42115/diff/1/10#newcode1258 Line 1258: REPLACEMENT_SUBSTRING = -5 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. http://codereview.chromium.org/42115/diff/1/10#newcode1267 Line 1267: int data; 'data' is too generic a name, there's no comment to explain what it's for. http://codereview.chromium.org/42115/diff/1/10#newcode1297 Line 1297: parts->Add(ReplacementPart(last, i)); Perhaps parts->Add could check for last == i to save you doing it in 4 out of 5 places. http://codereview.chromium.org/42115/diff/1/10#newcode1319 Line 1319: case '0': Are $0 and $04 allowed? Should $0 be taken verbatim? http://codereview.chromium.org/42115/diff/1/10#newcode1356 Line 1356: default: Should $F00 replace with $FOO or FOO? http://codereview.chromium.org/42115/diff/1/10#newcode1435 Line 1435: FixedArray* match_data = last_match_info_handle->elements(); Not using a handle is a little dodgy. If you move this outside the loop you don't have to worry about the Handle copying overhead. http://codereview.chromium.org/42115/diff/1/10#newcode1504 Line 1504: CONVERT_CHECKED(JSArray, last_match_info, args[3]); You need to check here that the last_match_info has fast elements and you need to check that is is big enough somewhere. 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, "", ""]; Do we still need both of these? http://codereview.chromium.org/42115/diff/1/12#newcode254 Line 254: // the result. Do we still need this? http://codereview.chromium.org/42115 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
