On 2011/08/31 14:14:13, Erik Corry wrote:
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.
Changed a few things according to suggestion and added a cctest to test
slice of
slice.
http://codereview.chromium.org/7795018/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
