Another bunch of comments. Once again I only looked on
architecture-independent
parts and on ia32 code.
http://codereview.chromium.org/7477045/diff/39001/src/handles.h
File src/handles.h (right):
http://codereview.chromium.org/7477045/diff/39001/src/handles.h#newcode188
src/handles.h:188:
nit: Undo blank line insertion.
http://codereview.chromium.org/7477045/diff/39001/src/heap-inl.h
File src/heap-inl.h (right):
http://codereview.chromium.org/7477045/diff/39001/src/heap-inl.h#newcode324
src/heap-inl.h:324: ASSERT(((type & kIsIndirectStringMask) ==
kIsIndirectStringTag)
The assert is extremely hard to read. Maybe we can drop it as an
indirect string is supposed to have a pointer to a "direct" string, so
it shouldn't matter what exact types of indirect strings there are.
http://codereview.chromium.org/7477045/diff/39001/src/heap.cc
File src/heap.cc (right):
http://codereview.chromium.org/7477045/diff/39001/src/heap.cc#newcode2552
src/heap.cc:2552: // Note that both parts cannot be string slices since:
"Note that neither of the two inputs can be a slice because:"
http://codereview.chromium.org/7477045/diff/39001/src/heap.cc#newcode2648
src/heap.cc:2648: !ConsString::cast(buffer)->first()->IsSeqString())) ||
On 2011/08/17 09:34:37, Yang wrote:
A little ugly for now, but this will be removed once I dealt with the
externalized string issue. For now externalized strings won't be
sliced, but
copied.
OK, but the indentation looks weird. Also please mark places that
require more work with TODO(slices bug number).
http://codereview.chromium.org/7477045/diff/39001/src/heap.cc#newcode2653
src/heap.cc:2653: ? AllocateRawAsciiString(length, pretenure )
nit: Extra space before ')'.
http://codereview.chromium.org/7477045/diff/39001/src/heap.cc#newcode2681
src/heap.cc:2681: MaybeObject* maybe_result = Allocate(map, NEW_SPACE);
Consider adding Heap::Allocate{Ascii,TwoByte}SlicedString.
http://codereview.chromium.org/7477045/diff/39001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
http://codereview.chromium.org/7477045/diff/39001/src/ia32/code-stubs-ia32.cc#newcode3405
src/ia32/code-stubs-ia32.cc:3405: __ j(zero, &seq_two_byte_string);
Looks like this code was never updated to use near labels...
http://codereview.chromium.org/7477045/diff/39001/src/ia32/code-stubs-ia32.cc#newcode3432
src/ia32/code-stubs-ia32.cc:3432: __ mov(ebx, FieldOperand(eax,
ConsString::kSecondOffset));
These two instructions can be combined into a single memory-immediate
compare.
http://codereview.chromium.org/7477045/diff/39001/src/ia32/code-stubs-ia32.cc#newcode3524
src/ia32/code-stubs-ia32.cc:3524: __ cmp(edi, kNotAStringSlice);
Will this still work if we initialize edi to 0 or to the slice offset
and unconditionally add it to ebx and the length loaded from the
original string?
http://codereview.chromium.org/7477045/diff/39001/src/ia32/code-stubs-ia32.cc#newcode5665
src/ia32/code-stubs-ia32.cc:5665: if (FLAG_string_slices) {
Whooops.
http://codereview.chromium.org/7477045/diff/39001/src/ia32/regexp-macro-assembler-ia32.cc
File src/ia32/regexp-macro-assembler-ia32.cc (right):
http://codereview.chromium.org/7477045/diff/39001/src/ia32/regexp-macro-assembler-ia32.cc#newcode1068
src/ia32/regexp-macro-assembler-ia32.cc:1068: HandleScope handles;
While you're here, please pass isolate to the scope constructor to avoid
a TLS load.
http://codereview.chromium.org/7477045/diff/39001/src/jsregexp.cc
File src/jsregexp.cc (left):
http://codereview.chromium.org/7477045/diff/39001/src/jsregexp.cc#oldcode487
src/jsregexp.cc:487: subject =
Handle<String>(ConsString::cast(*subject)->first(), isolate);
This unwrapping didn't help the code called below because it can handle
flat conses, right? What about interpreted regexps? I think the encoding
can now be incorrect for them.
http://codereview.chromium.org/7477045/diff/39001/src/jsregexp.cc
File src/jsregexp.cc (right):
http://codereview.chromium.org/7477045/diff/39001/src/jsregexp.cc#newcode239
src/jsregexp.cc:239: // Extract flattened substrings of cons strings
before determining asciiness.
Update the comment.
http://codereview.chromium.org/7477045/diff/39001/src/jsregexp.cc#newcode255
src/jsregexp.cc:255: subject->ToAsciiVector(),
To{Ascii,UC16}Vector won't work in case an indirect string encoding
disagrees with its underlying string encoding.
http://codereview.chromium.org/7477045/diff/39001/src/objects-debug.cc
File src/objects-debug.cc (right):
http://codereview.chromium.org/7477045/diff/39001/src/objects-debug.cc#newcode374
src/objects-debug.cc:374: CHECK(!this->parent()->IsSlicedString());
Add a min length check?
http://codereview.chromium.org/7477045/diff/39001/src/objects-inl.h
File src/objects-inl.h (right):
http://codereview.chromium.org/7477045/diff/39001/src/objects-inl.h#newcode2196
src/objects-inl.h:2196: return ConsString::cast(this)->second() ==
GetHeap()->empty_string();
Length check is probably a tiny bit faster as GetHeap is not cheap.
http://codereview.chromium.org/7477045/diff/39001/src/objects-inl.h#newcode2206
src/objects-inl.h:2206: return
String::cast(reinterpret_cast<ConsString*>(this)->first());
Doesn't work as advertised in the .h file. It should also support
slices.
http://codereview.chromium.org/7477045/diff/39001/src/objects.cc
File src/objects.cc (right):
http://codereview.chromium.org/7477045/diff/39001/src/objects.cc#newcode744
src/objects.cc:744: ConsString* cs = ConsString::cast(this);
If there are no intended semantic changes here, please revert and keep
the old version.
http://codereview.chromium.org/7477045/diff/39001/src/objects.cc#newcode5248
src/objects.cc:5248: // Note that the parent of a slice cannot be a
cons.
... or a slice.
http://codereview.chromium.org/7477045/diff/39001/src/objects.cc#newcode5251
src/objects.cc:5251: string = slice->parent();
string_tag also should be updated. Either add a TODO or fix now.
http://codereview.chromium.org/7477045/diff/39001/src/objects.cc#newcode5281
src/objects.cc:5281: // Note that the parent of a slice cannot be a
cons.
See above.
http://codereview.chromium.org/7477045/diff/39001/src/objects.h
File src/objects.h (right):
http://codereview.chromium.org/7477045/diff/39001/src/objects.h#newcode5817
src/objects.h:5817:
nit: Remove one blank line.
http://codereview.chromium.org/7477045/diff/39001/src/objects.h#newcode5820
src/objects.h:5820: inline String* GetIndirect();
GetUnderlying? GetIndirect sounds as if it should return an indirect
string.
http://codereview.chromium.org/7477045/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
