LGTM
http://codereview.chromium.org/2596001/diff/1/5 File src/ia32/codegen-ia32.cc (right): http://codereview.chromium.org/2596001/diff/1/5#newcode11292 src/ia32/codegen-ia32.cc:11292: ASSERT((kStringTag | kSeqStringTag | kAsciiStringTag) == 0); Consider using STATIC_ASSERT where possible. http://codereview.chromium.org/2596001/diff/1/5#newcode12840 src/ia32/codegen-ia32.cc:12840: ASSERT(0 == kAsciiStringTag); Asserting it once in four lines should be sufficient. http://codereview.chromium.org/2596001/diff/1/5#newcode12873 src/ia32/codegen-ia32.cc:12873: ASSERT((kConsStringTag &kExternalStringTag) == 0); Missing space after '&' http://codereview.chromium.org/2596001/diff/1/6 File src/ia32/macro-assembler-ia32.cc (right): http://codereview.chromium.org/2596001/diff/1/6#newcode1547 src/ia32/macro-assembler-ia32.cc:1547: Register scratch, Feel free to remove the scratch register from the signature. http://codereview.chromium.org/2596001/diff/1/6#newcode1573 src/ia32/macro-assembler-ia32.cc:1573: mov(scratch1, FieldOperand(object1, HeapObject::kMapOffset)); You could OR the instance types and only TEST the result, so you only need one conditional jump. http://codereview.chromium.org/2596001/diff/1/7 File src/objects.h (right): http://codereview.chromium.org/2596001/diff/1/7#newcode430 src/objects.h:430: SYMBOL_TYPE = kTwoByteStringTag | kSymbolTag | kSeqStringTag, Good change! At some point, we ought to insert TWOBYTE_ into the names of the ones that aren't ASCII. It can be confusing that two-byte strings appear to be the default, with ASCII strings being the exception. http://codereview.chromium.org/2596001/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
