mul-exhaustive.js should be simplified. It seems too complicated and it's
not
obvious why it has to be.
http://codereview.chromium.org/6688062/diff/1/test/mjsunit/math-sqrt.js
File test/mjsunit/math-sqrt.js (right):
http://codereview.chromium.org/6688062/diff/1/test/mjsunit/math-sqrt.js#newcode33
test/mjsunit/math-sqrt.js:33: // Math.pow(-0, 0.5) must be zero, but
Math.sqrt(-0) is -0.
I think this should be converted from a comment into a test.
http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mjsunit.js
File test/mjsunit/mjsunit.js (right):
http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mjsunit.js#newcode44
test/mjsunit/mjsunit.js:44: function MJSToString(value) {
You should adopt the same naming convention as the surrounding code or
else change the surrounding code (MjsToString, see MjsUnitAssertionError
just above, and I think it should properly be MjsUnitToString---"mini
JSUnit" is the testing framework, I don't know what "mini JS" is).
http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mjsunit.js#newcode82
test/mjsunit/mjsunit.js:82: var start;
I can't figure out why this function is so convoluted.
http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js
File test/mjsunit/mul-exhaustive.js (right):
http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js#newcode33
test/mjsunit/mul-exhaustive.js:33: // Test multiplication where both
operands are variables.
It's hard to reverse engineer what the the new parts of this test are
doing. The cryptic identifier names don't help.
This comment does not help at all, because "-x" is not a variable.
Please just drop this comment.
Please rename "a" to "expected".
http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js#newcode43
test/mjsunit/mul-exhaustive.js:43: // Test multiplication where either
or both operands are constant.
"Constant" is ambiguous (especially if contrasted with "variable", since
we have const in the language). Use "literals".
http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js#newcode46
test/mjsunit/mul-exhaustive.js:46: var xs = String(x);
It might be more straightforward to skip converting both x and y and
both their negations to strings by defining a helper
function stringify(n) { return (1 / n === -Infinity) ? '-0' : String(n);
}
http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js#newcode53
test/mjsunit/mul-exhaustive.js:53: function fs(a, x, y, xs, ys) {
Here I think you could probably find a way to fold the earlier asserts
into this function. You could also convert x and y to strings on
demand. I'm thinking something like:
function subtest(expected, x, y) {
assertEquals(expected, x * y);
assertEquals(expected, eval(stringify(x) + '*y'));
assertEquals(expected, eval('x*' + stringify(y));
assertEquals(expected, eval(stringify(x) + '*' + stringify(y));
}
subtest(expected, x, y);
subtest(-expected, -x, -y);
subtext(-expected, x, -y);
// etc.
http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js#newcode55
test/mjsunit/mul-exhaustive.js:55: for (var i = 0; i < 1000; i++)
assertEquals(a, xcyv(y));
I don't understand the purpose of the loop. If it's to hit the
optimizing compiler, I think you'll need a bigger constant.
http://codereview.chromium.org/6688062/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
