LGTM, but do we have enough test coverage?  test/mjsunit/abs.js is somewhat
small, for example it apparently doesn't test the case of the smallest Smi.


http://codereview.chromium.org/3446024/diff/1/2
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/3446024/diff/1/2#newcode1578
src/ia32/macro-assembler-ia32.cc:1578: psllq(dst, 2 * kBitsPerInt - 1);
I wonder if 2 * kBitsPerInt - 1 is a proper way to encode 63 here :)
but up to you

http://codereview.chromium.org/3446024/diff/1/4
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/3446024/diff/1/4#newcode1967
src/ia32/stub-cache-ia32.cc:1967: if (!object->IsJSObject() || argc !=
1) return Heap::undefined_value();
pretty minor thing: maybe you shouldn't bail out if argc != 1 and only
process first argument?  That's apparently what the spec requires

http://codereview.chromium.org/3446024/show

--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev

Reply via email to