LGTM with comments.
http://codereview.chromium.org/2131019/diff/1/2 File src/runtime.js (right): http://codereview.chromium.org/2131019/diff/1/2#newcode566 src/runtime.js:566: if (x === 0 && y === 0 && !%_IsSmi(x) && !%_IsSmi(y) && Can this be right? They both have to be non-Smi to get into this 'if'? http://codereview.chromium.org/2131019/diff/1/2#newcode573 src/runtime.js:573: if (IS_BOOLEAN(x))return y == x; Missing space after ) http://codereview.chromium.org/2131019/diff/1/3 File src/v8natives.js (right): http://codereview.chromium.org/2131019/diff/1/3#newcode545 src/v8natives.js:545: // Step 5 Step 5 seems to be a part of Step 6, so you can just omit it, right? http://codereview.chromium.org/2131019/diff/1/3#newcode562 src/v8natives.js:562: SameValue(desc.getSet(), current.getSet()))) return true; Multiline 'if's should use {}. http://codereview.chromium.org/2131019/diff/1/4 File test/mjsunit/regress/regress-712.js (right): http://codereview.chromium.org/2131019/diff/1/4#newcode28 test/mjsunit/regress/regress-712.js:28: // See: http://code.google.com/p/v8/issues/detail?id=712 Let's have a short description here too. It's annoying to have to look up and I can easily imagine the V8 source code outliving code.google.com URLs. http://codereview.chromium.org/2131019/diff/1/4#newcode35 test/mjsunit/regress/regress-712.js:35: assertEquals(obj.x, "42"); This test looks a bit minimal. Perhaps it should be a separate test, but I'd like to see a test designed to break out of the big 'step 6' if statement at every disjunction. Also it should test the +-0 line in the IsSame function. http://codereview.chromium.org/2131019/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
