LGTM, but there are some nits to fix before committing.
The current commit message is a little inaccurate, (there are no changes in
this
CL to class_fmt, or min_max (though I asked for naming changes in
min_max.), and
the 2nd line seems completely redundant with the title.
I suggest:
MIPS: Fix mina_maxa for proper NaN handling.
Also clean up variable naming in min_max.
https://codereview.chromium.org/1276813004/diff/1/test/cctest/test-assembler-mips.cc
File test/cctest/test-assembler-mips.cc (right):
https://codereview.chromium.org/1276813004/diff/1/test/cctest/test-assembler-mips.cc#newcode1805
test/cctest/test-assembler-mips.cc:1805: const int tableLength = 15;
nit: name capitalization does not conform to style guide. Since it's a
const, I suggest kTableLength
https://codereview.chromium.org/1276813004/diff/1/test/cctest/test-assembler-mips.cc#newcode1811
test/cctest/test-assembler-mips.cc:1811: const float fltNaN =
std::numeric_limits<float>::quiet_NaN();
nit: these two names also don't conform. Suggest dbl_nan, and flt_nan. I
actually prefer spelling them out, since 'flt' isn't so mnemonic to me:
float_nan, double_nan.
It would be nice if you fix the similar names in TEST(min_max) as well.
https://codereview.chromium.org/1276813004/diff/1/test/cctest/test-assembler-mips64.cc
File test/cctest/test-assembler-mips64.cc (right):
https://codereview.chromium.org/1276813004/diff/1/test/cctest/test-assembler-mips64.cc#newcode1907
test/cctest/test-assembler-mips64.cc:1907: const int tableLength = 15;
Same comment as in 32-bit versionL kTableLength, double_nan, and
float_nan.
https://codereview.chromium.org/1276813004/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
