LGTM
http://codereview.chromium.org/7491054/diff/1/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right): http://codereview.chromium.org/7491054/diff/1/src/arm/code-stubs-arm.cc#newcode1613 src/arm/code-stubs-arm.cc:1613: const Register map = r9.is(tos_) ? r7 : r9; If we have ever seen an internal object you could start with a Smi check and an internal object check here. Then you could remove the special handling for internal object on the other 10 cases. It would be slightly slower for the internal object case, but that is rare and arguably broken anyway. It would simplify things a lot. http://codereview.chromium.org/7491054/diff/1/src/arm/code-stubs-arm.cc#newcode1615 src/arm/code-stubs-arm.cc:1615: // undefined -> false Missing full stops on the end of comments several places. Not your fault, but we might as well fix it now that we are looking at the code. http://codereview.chromium.org/7491054/diff/1/src/arm/code-stubs-arm.cc#newcode1638 src/arm/code-stubs-arm.cc:1638: // Everything with a map could be undetectable, so check this now. Pretty sure that it is just strings and JSObjects. The tests will catch it if I am wrong, at least they will if they run everything twice, one to patch the ToBooleanStub and the second time to run it. http://codereview.chromium.org/7491054/diff/1/src/arm/code-stubs-arm.cc#newcode1689 src/arm/code-stubs-arm.cc:1689: // internal objects -> true Caps and full stop. http://codereview.chromium.org/7491054/diff/1/src/arm/lithium-arm.cc File src/arm/lithium-arm.cc (right): http://codereview.chromium.org/7491054/diff/1/src/arm/lithium-arm.cc#newcode1044 src/arm/lithium-arm.cc:1044: // environment then. Do we hit this case in the tests? I would guess it is so rare that the best way may be not to special case it. http://codereview.chromium.org/7491054/diff/1/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): http://codereview.chromium.org/7491054/diff/1/src/arm/lithium-codegen-arm.cc#newcode1589 src/arm/lithium-codegen-arm.cc:1589: // We've seen undefined for the first time -> deopt. We should just bail out if we have seen an internal object. http://codereview.chromium.org/7491054/diff/1/src/arm/lithium-codegen-arm.cc#newcode1594 src/arm/lithium-codegen-arm.cc:1594: if (expected.Contains(ToBooleanStub::BOOLEAN)) { This 'if' and the next can be merged. http://codereview.chromium.org/7491054/diff/1/src/arm/lithium-codegen-arm.cc#newcode1601 src/arm/lithium-codegen-arm.cc:1601: __ LoadRoot(ip, Heap::kTrueValueRootIndex); We have CompareRoot in the macro assembler. http://codereview.chromium.org/7491054/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
