LGTM

http://codereview.chromium.org/3327022/diff/3001/4004
File src/ia32/disasm-ia32.cc (right):

http://codereview.chromium.org/3327022/diff/3001/4004#newcode1163
src/ia32/disasm-ia32.cc:1163: int8_t imm8 =
*reinterpret_cast<int8_t*>(data + 1);
Why not just:
int imm8 = data[1];
?

http://codereview.chromium.org/3327022/diff/3001/4007
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/3327022/diff/3001/4007#newcode1871
src/ia32/stub-cache-ia32.cc:1871: // discards NaN.
It might be nice to detect +- zero and pass it through unchanged.  Zero
is probably reasonably common.  Once you have done that then negative
numbers are perhaps not so hard?  It seems like it's the same except you
want to subtract then add instead of adding then subtracting below and
then you need to reverse the rest too.  You can save this for another
change or not do it at all :-)

http://codereview.chromium.org/3327022/diff/3001/4010
File test/mjsunit/math-floor.js (right):

http://codereview.chromium.org/3327022/diff/3001/4010#newcode31
test/mjsunit/math-floor.js:31: assertEquals(0, Math.floor(0));
This tests the Smi 0 but not the heap number 0.  Normally + will not
check for a Smi result so this should work:

x = 0.5
y = 0.5
assertEquals(x - y, Math.floor(x - y)))

http://codereview.chromium.org/3327022/diff/3001/4010#newcode32
test/mjsunit/math-floor.js:32: assertEquals(-0, Math.floor(-0));
0 == -0 so this test doesn't really work.  You need to do

1/-0, 1/Math.floor(-0)

http://codereview.chromium.org/3327022/diff/3001/4010#newcode60
test/mjsunit/math-floor.js:60: var two_30 = 1 << 30;
It would be nice to pass this in as an argument so you could easily do
two_53 as well.

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

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

Reply via email to