Do we have adequate test coverage? Cases that should be covered include:
One value is zero, other value is negative, positive, zero.
Values that would overflow like the example in the comments, 1e9 and 9.
All pairs on the power-of-10 borders like 99 vs. 100, 999 vs. 1000.
Max positive and negative ints and Smis together with a selection of other
ints.
Please test on x64 where Smis are 32 bit.
LGTM with either tests or a pointer to an existing mjsunit test that covers
all
this.
http://codereview.chromium.org/7261008/diff/1/src/misc-intrinsics.h
File src/misc-intrinsics.h (right):
http://codereview.chromium.org/7261008/diff/1/src/misc-intrinsics.h#newcode42
src/misc-intrinsics.h:42: } } // namespace v8::internal
Instead of closing the name space here and then using explicit name
spaces on the definitions below you can just close it at the end.
http://codereview.chromium.org/7261008/diff/1/src/misc-intrinsics.h#newcode67
src/misc-intrinsics.h:67: shift = (value > 0xFFFF) << 4; value >>=
shift; result = shift;
This code won't pass presubmit.py. See
http://code.google.com/p/v8/wiki/Contributing
Also, we don't normally allow booleans to be used in a numeric way.
Please replace with ?:
http://codereview.chromium.org/7261008/diff/1/src/runtime.cc
File src/runtime.cc (right):
http://codereview.chromium.org/7261008/diff/1/src/runtime.cc#newcode6665
src/runtime.cc:6665: static const int powersOf10[] = { 1, 10, 100, 1000,
10*1000, 100*1000,
This is a constant so it should be named as a constant: kPowersOf10.
Also, the indentation is wrong. Newline-after { is the norm in the rest
of the code base and } gets its own line.
http://codereview.chromium.org/7261008/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
