Thanks for review! Comments addressed, proceeding with platform ports
https://codereview.chromium.org/101413006/diff/20001/src/elements-kind.h
File src/elements-kind.h (right):
https://codereview.chromium.org/101413006/diff/20001/src/elements-kind.h#newcode151
src/elements-kind.h:151: IsFixedFloatOrDoubleElementsKind(kind);
On 2013/12/23 10:40:32, Toon Verwaest wrote:
What about just renaming this to IsFixedFloatElementsKind, given that
now you
called them both Float[32|64]?
Done.
https://codereview.chromium.org/101413006/diff/20001/src/heap.cc
File src/heap.cc (right):
https://codereview.chromium.org/101413006/diff/20001/src/heap.cc#newcode2827
src/heap.cc:2827: if (!maybe_obj->ToObject(&obj)) return false;
Done (+ more boilerplate scratching)
On 2013/12/23 10:40:32, Toon Verwaest wrote:
Can we already change all these ToObject to To, only declare maybe_obj
once; and
remove all the { ... }? And probably make "obj" instead Map* map; so
that we
don't need to Map::cast obj. Hence:
Map* map;
MaybeObject* maybe_map;
...
maybe_map = AllocateMap(FIXED_INT8_ARRAY_TYPE, kVariableSizeSentinel);
if (!maybe_map->To(&map)) return false;
set_fixed_int8_array_map(map);
...
I think all this syntactical noise is getting more and more
ridiculous.
https://codereview.chromium.org/101413006/diff/20001/src/heap.cc#newcode4349
src/heap.cc:4349: int element_size = 1; // Bogus.
On 2013/12/23 10:40:32, Toon Verwaest wrote:
Doesn't it work to set element_size and elements_kind in the
UNREACHABLE()
branch above, rather than initialize them here? Then you don't need
those //
Bogus labels here and it's in sync with what most other code does.
Done.
https://codereview.chromium.org/101413006/diff/20001/src/heap.cc#newcode4356
src/heap.cc:4356: { MaybeObject* maybe_object = AllocateRaw(size, space,
OLD_DATA_SPACE);
On 2013/12/23 10:40:32, Toon Verwaest wrote:
Remove the { ... }.
Done.
https://codereview.chromium.org/101413006/diff/20001/src/heap.cc#newcode4357
src/heap.cc:4357: if (!maybe_object->To<HeapObject>(&object)) return
maybe_object;
On 2013/12/23 10:40:32, Toon Verwaest wrote:
Is the <HeapObject> really necessary? If not remove it. Otherwise, if
it's
necessary cause the map isn't correct yet, can we directly cast to
FixedTypedArrayBase?
No because FixedTypedArrayBase::cast checks map as well.
https://codereview.chromium.org/101413006/diff/20001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/101413006/diff/20001/src/hydrogen-instructions.h#newcode6441
src/hydrogen-instructions.h:6441: SetGVNFlag(kDependsOnArrayElements);
On 2013/12/23 10:40:32, Toon Verwaest wrote:
Don't you want to make a separate DependsOnTypedArrayElements?
Done. Good suggestion.
https://codereview.chromium.org/101413006/diff/20001/src/hydrogen-instructions.h#newcode6728
src/hydrogen-instructions.h:6728: ? Representation::Integer32()
On 2013/12/23 10:40:32, Toon Verwaest wrote:
4-space indent if you put the ? on a new line.
Done.
https://codereview.chromium.org/101413006/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/101413006/diff/20001/src/hydrogen.cc#newcode2040
src/hydrogen.cc:2040: && !IsFixedTypedArrayElementsKind(elements_kind))
On 2013/12/23 10:40:32, Toon Verwaest wrote:
&& on previous line. Align new line 1 space further than is currently
the case.
Done.
https://codereview.chromium.org/101413006/diff/20001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):
https://codereview.chromium.org/101413006/diff/20001/src/ia32/lithium-ia32.cc#newcode2358
src/ia32/lithium-ia32.cc:2358:
instr->elements()->representation().IsTagged()) ||
On 2013/12/23 10:40:32, Toon Verwaest wrote:
Align the instr at the same level of indentation.
Done.
https://codereview.chromium.org/101413006/diff/20001/src/ia32/lithium-ia32.cc#newcode2369
src/ia32/lithium-ia32.cc:2369: return new(zone())
LStoreKeyed(backing_store,
On 2013/12/23 10:40:32, Toon Verwaest wrote:
I'd put backing_store, key, val); on a single line.
Done.
https://codereview.chromium.org/101413006/diff/20001/src/objects-debug.cc
File src/objects-debug.cc (right):
https://codereview.chromium.org/101413006/diff/20001/src/objects-debug.cc#newcode340
src/objects-debug.cc:340: &&
HeapObject::cast(this)->map()->instance_type()
On 2013/12/23 10:40:32, Toon Verwaest wrote:
&& on previous line. Also ==. Indent +4 spaces for the line after ==.
Done.
https://codereview.chromium.org/101413006/diff/20001/src/objects-inl.h
File src/objects-inl.h (right):
https://codereview.chromium.org/101413006/diff/20001/src/objects-inl.h#newcode145
src/objects-inl.h:145: || IsFixedTypedArrayBase() || IsExternalArray();
On 2013/12/23 10:40:32, Toon Verwaest wrote:
|| on previous line.
Done.
https://codereview.chromium.org/101413006/diff/20001/src/objects-inl.h#newcode274
src/objects-inl.h:274: || IsFixedTypedArrayBase();
On 2013/12/23 10:40:32, Toon Verwaest wrote:
|| on previous line.
Done.
https://codereview.chromium.org/101413006/diff/20001/src/objects-inl.h#newcode3573
src/objects-inl.h:3573: // Clamp undefined to zero (default). All other
types have been
On 2013/12/23 10:40:32, Toon Verwaest wrote:
I'd just write "Clamp undefined to the default value"; rather than
specifying
here that that's zero.
Done.
https://codereview.chromium.org/101413006/diff/20001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/101413006/diff/20001/src/objects.h#newcode4914
src/objects.h:4914:
On 2013/12/23 10:40:32, Toon Verwaest wrote:
Double newlines between classes.
This is a lot of boilerplate code...
Done.
https://codereview.chromium.org/101413006/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):
https://codereview.chromium.org/101413006/diff/20001/test/cctest/test-api.cc#newcode16186
test/cctest/test-api.cc:16186:
factory->NewFixedTypedArray(kElementCount,
On 2013/12/23 10:40:32, Toon Verwaest wrote:
Join lines
Done.
https://codereview.chromium.org/101413006/diff/20001/test/cctest/test-api.cc#newcode16194
test/cctest/test-api.cc:16194: }
Yes. 'set' just asserts on OOB
On 2013/12/23 10:40:32, Toon Verwaest wrote:
From what I can tell, out-of-bounds accesses are tested in
ObjectWithExternalArrayTestHelper?
https://codereview.chromium.org/101413006/diff/20001/test/cctest/test-api.cc#newcode16205
test/cctest/test-api.cc:16205: jsobj,
On 2013/12/23 10:40:32, Toon Verwaest wrote:
join lines
Done.
https://codereview.chromium.org/101413006/diff/20001/test/cctest/test-api.cc#newcode16211
test/cctest/test-api.cc:16211: context.local(),
On 2013/12/23 10:40:32, Toon Verwaest wrote:
Don't all these arguments fit on a single line?
Done.
https://codereview.chromium.org/101413006/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
