LGTM with a few fixes.
http://codereview.chromium.org/6682026/diff/1014/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right): http://codereview.chromium.org/6682026/diff/1014/src/arm/code-stubs-arm.cc#newcode6017 src/arm/code-stubs-arm.cc:6017: // Must be null (deleted entry). How about adding some debug code to test that it is null? http://codereview.chromium.org/6682026/diff/1014/src/arm/code-stubs-arm.cc#newcode6022 src/arm/code-stubs-arm.cc:6022: // Check that the candidate is a non-external ascii string. The instance ASCII is an acronym. http://codereview.chromium.org/6682026/diff/1014/src/arm/codegen-arm.cc File src/arm/codegen-arm.cc (right): http://codereview.chromium.org/6682026/diff/1014/src/arm/codegen-arm.cc#newcode7209 src/arm/codegen-arm.cc:7209: Add to the comment that we know the length is a smi because we tested the array to have fast elements above. http://codereview.chromium.org/6682026/diff/1014/src/arm/codegen-arm.cc#newcode7209 src/arm/codegen-arm.cc:7209: This code now clobbers scratch1 which is assumed to be the elements fixed-array below. http://codereview.chromium.org/6682026/diff/1014/src/ia32/full-codegen-ia32.cc File src/ia32/full-codegen-ia32.cc (right): http://codereview.chromium.org/6682026/diff/1014/src/ia32/full-codegen-ia32.cc#newcode3052 src/ia32/full-codegen-ia32.cc:3052: __ CmpObjectType(object, JS_OBJECT_TYPE, temp); JS_ARRAY_TYPE http://codereview.chromium.org/6682026/diff/1014/src/x64/assembler-x64.h File src/x64/assembler-x64.h (right): http://codereview.chromium.org/6682026/diff/1014/src/x64/assembler-x64.h#newcode659 src/x64/assembler-x64.h:659: // Instructions to load a 64-bit immediate into a register. Good change. Never name something "new X". It gets old fast :) http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64.cc File src/x64/macro-assembler-x64.cc (right): http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64.cc#newcode127 src/x64/macro-assembler-x64.cc:127: // catch stores of smis and stores into young gen. gen -> generation. http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64.cc#newcode156 src/x64/macro-assembler-x64.cc:156: // catch stores of smis and stores into young gen. ditto. http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64.cc#newcode843 src/x64/macro-assembler-x64.cc:843: AbortIfNotSmi(smi2); Good stuff! http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64.cc#newcode855 src/x64/macro-assembler-x64.cc:855: Cmp(dst, src); ... apart from the optimizations that Cmp also does. http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64.cc#newcode897 src/x64/macro-assembler-x64.cc:897: // The Operand cannot use the smi register, since we may use the scratch Notice that smi_reg is either kScratchRegister or kSmiConstantRegister in this case. http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64.cc#newcode1797 src/x64/macro-assembler-x64.cc:1797: Condition is_smi = CheckSmi(object); Negate condition? http://codereview.chromium.org/6682026/diff/1014/test/cctest/test-macro-assembler-x64.cc File test/cctest/test-macro-assembler-x64.cc (right): http://codereview.chromium.org/6682026/diff/1014/test/cctest/test-macro-assembler-x64.cc#newcode223 test/cctest/test-macro-assembler-x64.cc:223: __ cmpq(rcx, rcx); Really should be SmiCompare in this function. http://codereview.chromium.org/6682026/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
