LGTM
http://codereview.chromium.org/8635011/diff/1/src/api.cc File src/api.cc (right): http://codereview.chromium.org/8635011/diff/1/src/api.cc#newcode4563 src/api.cc:4563: if (size < i::ExternalString::kShortSize) Put braces around the then-statement. http://codereview.chromium.org/8635011/diff/1/src/ia32/codegen-ia32.cc File src/ia32/codegen-ia32.cc (right): http://codereview.chromium.org/8635011/diff/1/src/ia32/codegen-ia32.cc#newcode535 src/ia32/codegen-ia32.cc:535: __ mov(result, FieldOperand(string, HeapObject::kMapOffset)); We're absolutely sure that only strings reach here, right? http://codereview.chromium.org/8635011/diff/1/src/ia32/codegen-ia32.cc#newcode571 src/ia32/codegen-ia32.cc:571: __ test_b(scratch, kShortExternalStringTag); Test with kShortExternalStringMask instead of ...Tag. http://codereview.chromium.org/8635011/diff/1/src/objects.cc File src/objects.cc (right): http://codereview.chromium.org/8635011/diff/1/src/objects.cc#newcode958 src/objects.cc:958: ASSERT(size >= ExternalString::kShortSize); This assert can safely be dropped. http://codereview.chromium.org/8635011/diff/1/src/objects.cc#newcode983 src/objects.cc:983: ExternalTwoByteString::kHashFieldOffset); These two asserts are unnecessary, since ExternalAsciiString doesn't have a static kLengthOffset or kHashFieldOffset property. This is comparing String::kLengthOffset with itself. Since it's placed on String, it should be assumed to apply to all string subtypes. http://codereview.chromium.org/8635011/diff/1/src/objects.cc#newcode1013 src/objects.cc:1013: ASSERT(size >= ExternalString::kShortSize); Ditto. http://codereview.chromium.org/8635011/diff/1/src/objects.cc#newcode1029 src/objects.cc:1029: ExternalAsciiString::kHashFieldOffset); As above. http://codereview.chromium.org/8635011/diff/1/src/objects.cc#newcode1030 src/objects.cc:1030: if (is_symbol) self->Hash(); // Force regeneration of the hash value. Is there an explanation of how the hash can possibly change? It should be determined solely by the character codes and length of the string, both of which are retained by externalization. http://codereview.chromium.org/8635011/diff/1/src/objects.h File src/objects.h (right): http://codereview.chromium.org/8635011/diff/1/src/objects.h#newcode234 src/objects.h:234: V(EXTERNAL_ASCII_SYMBOL_TYPE) \ No SHORT_EXTERNAL_SYMBOL_TYPE etc.? http://codereview.chromium.org/8635011/diff/1/src/objects.h#newcode6829 src/objects.h:6829: inline void update_data_cache(); Put a comment saying that this function is only used by deserialization, and that the data cache is otherwise always set correctly http://codereview.chromium.org/8635011/diff/1/test/cctest/test-api.cc File test/cctest/test-api.cc (right): http://codereview.chromium.org/8635011/diff/1/test/cctest/test-api.cc#newcode494 test/cctest/test-api.cc:494: uint16_t* two_byte_string = AsciiToTwoByteString("small"); Whether the externalized version of this string is short or not depends on the size of pointers. On 64-bit it will be short, on 32-bit it won't. Maybe we should select a string based on kPointerSize instead, for tests like this, so it tests the same thing on all platforms. http://codereview.chromium.org/8635011/ -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev
