LGTM with comments.
http://codereview.chromium.org/618002/diff/1/3 File src/builtins.cc (right): http://codereview.chromium.org/618002/diff/1/3#newcode533 src/builtins.cc:533: // Not conformant, but that's the way it is currently. Could you use the comment from array.js which is more informative than this one? // SpiderMonkey and JSC return undefined in the case where no // arguments are given instead of using the implicit undefined // arguments. This does not follow ECMA-262, but we do the same for // compatibility. http://codereview.chromium.org/618002/diff/1/3#newcode534 src/builtins.cc:534: if (n_arguments == 0) Braces around the body or single-line. http://codereview.chromium.org/618002/diff/1/3#newcode548 src/builtins.cc:548: // but current implementation behaves differently. Again, please use the more informative comment from array.js for this one. http://codereview.chromium.org/618002/diff/1/5 File test/mjsunit/array-functions-prototype-2.js (right): http://codereview.chromium.org/618002/diff/1/5#newcode34 test/mjsunit/array-functions-prototype-2.js:34: var LARGE = 40000000; Please make sure that this test does not take a long time. This should be a simple test that can be run very quickly. Benchmarks are fine but should not be part of the mjsunit test suite. http://codereview.chromium.org/618002/diff/1/5#newcode37 test/mjsunit/array-functions-prototype-2.js:37: // Nicer for firefox 1.5. Unless you uncomment the following two lines, Let's remove this. This should test correctness and not performance. http://codereview.chromium.org/618002/diff/1/5#newcode145 test/mjsunit/array-functions-prototype-2.js:145: // IE, Safari get this right. We should just test what we believe is the right thing here. Let's remove the comments about what the other engines do. These comments are bound to become wrong when the other implementations change and all that matters is that we test the correct behavior. http://codereview.chromium.org/618002/diff/1/5#newcode200 test/mjsunit/array-functions-prototype-2.js:200: // IE, Safari get this wrong. Ditto. http://codereview.chromium.org/618002 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
