ARM part LGTM, but may have redundant code.
http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right): http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newcode5486 src/arm/code-stubs-arm.cc:5486: // Special handling of sub-strings of length 1 and 2. One character strings This comment is out of date, since the code handles substrings of length 2 without going into the runtime system. http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newcode5508 src/arm/code-stubs-arm.cc:5508: __ b(ge, &return_r0); I think eq is clearer than ge. A comment to the effect that r2 is the length would be nice. http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newcode5527 src/arm/code-stubs-arm.cc:5527: __ b(gt, &runtime); // External strings go to runtime. Update comment to say that external and sliced strings go to runtime. http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newcode5538 src/arm/code-stubs-arm.cc:5538: __ b(ne, &runtime); // Cons and External strings go to runtime. and slices, or can they not be found under a cons string? http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newcode5546 src/arm/code-stubs-arm.cc:5546: // r3: from index (untaged smi) untaged -> untagged also at least one other place. http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newcode5554 src/arm/code-stubs-arm.cc:5554: // r0: original string Original string or left hand side of the original cons string. http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newcode5600 src/arm/code-stubs-arm.cc:5600: // r5: first character of sub string to copy sub string -> substring http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newcode5623 src/arm/code-stubs-arm.cc:5623: // byte character. Add a static assert that the kSmiTag is 0 and kSmiTagSize is 1. http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newcode5631 src/arm/code-stubs-arm.cc:5631: // r5: first character of sub string to copy sub string -> substring http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newcode5666 src/arm/code-stubs-arm.cc:5666: // Cons string. Check whether it is flat, then fetch first part. How do we get here if it's a cons string? http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newcode5675 src/arm/code-stubs-arm.cc:5675: // Sliced string. Fetch parent and correct start index by offset. Do you have test coverage here? I think we already bailed to runtime in this case. http://codereview.chromium.org/7795018/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
