LGTM.
Obviously it would be great to have it work in generated code on X64 and
ARM
too, but since this is a safe extension, the current code will work until
the
optimization is ported. Nice!
http://codereview.chromium.org/8513010/diff/1012/src/code-stubs.h
File src/code-stubs.h (right):
http://codereview.chromium.org/8513010/diff/1012/src/code-stubs.h#newcode798
src/code-stubs.h:798: // result character. We expect |index| to be not
smi-tagged.
"not smi-tagged" -> "untagged".
http://codereview.chromium.org/8513010/diff/1012/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
http://codereview.chromium.org/8513010/diff/1012/src/ia32/code-stubs-ia32.cc#newcode5156
src/ia32/code-stubs-ia32.cc:5156: __ Assert(zero, "external string
expected, but not found");
This test checks for "indirect string" but reports "not external". Seems
to ignore sequential strings. Make a comment here if we ruled those out
earlier.
http://codereview.chromium.org/8513010/diff/1012/src/ia32/code-stubs-ia32.cc#newcode5164
src/ia32/code-stubs-ia32.cc:5164: __ cmp(FieldOperand(string,
HeapObject::kMapOffset),
Comparing against the same memory location twice in a row. Is there no
register free to load it?
Or, possibly, keep it alive since we first loaded it in line 5129?
But this'll probably not happen often enough to warrant a scratch
register.
http://codereview.chromium.org/8513010/diff/1012/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
http://codereview.chromium.org/8513010/diff/1012/src/ia32/lithium-codegen-ia32.cc#newcode3377
src/ia32/lithium-codegen-ia32.cc:3377:
StringCharCodeAtGenerator::GenerateCharLoad(masm(),
If GenerateCharLoad is used from outside StringCharCodeAtGenerator, you
might as well make it a macro in macro-assembler-*.
http://codereview.chromium.org/8513010/diff/1012/src/objects-inl.h
File src/objects-inl.h (right):
http://codereview.chromium.org/8513010/diff/1012/src/objects-inl.h#newcode2321
src/objects-inl.h:2321: if (*data_field == NULL) *data_field =
resource()->data();
Alternatively, we could ensure that the data_field is always set (by
setting it when creating the string object). That allows us to avoid
checking it on every access.
http://codereview.chromium.org/8513010/diff/1012/src/objects.cc
File src/objects.cc (right):
http://codereview.chromium.org/8513010/diff/1012/src/objects.cc#newcode6056
src/objects.cc:6056: return GetChars() + start;
Functions like this are so small, it would be great if they could be
inlined, i.e., if they were in objects-inl.h instead.
http://codereview.chromium.org/8513010/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
