http://codereview.chromium.org/9638014/diff/3005/src/elements.cc File src/elements.cc (right):
http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode65 src/elements.cc:65: // identical. Note that the order must match that of the ElementsKind enum for I think it would be nice, but it's a bit beyond the scope of this CL. On 2012/03/09 10:09:36, Jakob wrote:
It sure would be nice to enforce this matching order, e.g. by using
this macro
to generate the enum... but I realize that header dependencies might
make that
difficult.
http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode148 src/elements.cc:148: if (copy_size == -1) copy_size = from_obj->length() - from_start; On 2012/03/09 10:09:36, Jakob wrote:
I'd move this line up so that the auto-detected copy_size is still
covered by
the ASSERT((copy_size + static_cast<int>(from_start)) <=
from_obj->length()).
As an additional sanity check, you could add ASSERT(copy_size >= 0),
but I guess
it's unlikely that would ever be triggered.
Done. http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode209 src/elements.cc:209: if (copy_size == -1) copy_size = from_obj->length() - from_start; On 2012/03/09 10:09:36, Jakob wrote:
I'd move this up too (see my other comment above).
Done. http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode228 src/elements.cc:228: CHECK(maybe_value_object->ToObject(&value)); On 2012/03/09 10:09:36, Jakob wrote:
What if the old space needs a GC? I think you want to return the
failure object
in this case instead of just CHECKing it.
Done. http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode245 src/elements.cc:245: if (copy_size == -1) copy_size = from_obj->length() - from_start; On 2012/03/09 10:09:36, Jakob wrote:
And once more, I'd move this up.
Done. http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode610 src/elements.cc:610: static MaybeObject* CopyElementsImpl(FixedArrayBase* from, The allocation of HeapNumbers can fail on the Double -> Fast conversion, so this has to be MaybeObject. On 2012/03/09 08:15:18, Sven Panne wrote:
This could return Object*, I think, allocations are not allowed
anyway. OTOH, my
mental compiler might overlook places where an actual failure could
arise... http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode695 src/elements.cc:695: to_start, copy_size); On 2012/03/09 10:09:36, Jakob wrote:
nit: indentation
Done. http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode989 src/elements.cc:989: return from; default: On 2012/03/09 08:15:18, Sven Panne wrote:
Layout...
Done. http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode989 src/elements.cc:989: return from; default: On 2012/03/09 08:15:18, Sven Panne wrote:
Layout...
Done. http://codereview.chromium.org/9638014/diff/3005/src/elements.cc#newcode1052 src/elements.cc:1052: NonStrictArgumentsElementsAccessor, On 2012/03/09 10:09:36, Jakob wrote:
very nitty nit: 4 spaces indent, not 5 :-)
Done. http://codereview.chromium.org/9638014/diff/3005/src/elements.h File src/elements.h (right): http://codereview.chromium.org/9638014/diff/3005/src/elements.h#newcode97 src/elements.h:97: FixedArrayBase* from = NULL) = 0; On 2012/03/09 08:15:18, Sven Panne wrote:
Reading only this header, it is a bit confusing why there is "from" in
addition
to "from_holder". A comment for a method with more than half a dozen
parameter
wouldn't hurt... ;-)
Done. http://codereview.chromium.org/9638014/diff/3005/src/elements.h#newcode145 src/elements.h:145: void CopyObjectToObjectElements(AssertNoAllocation* no_gc, On 2012/03/09 08:15:18, Sven Panne wrote:
Hmmm, the first parameter is actually a phantom type, I guess. We have
one other
such type in v8 I know of (WhitenessWitness), but I have the gut
feeling that
passing an instance of such phantom types around at runtime is very
inelegant.
What we really want is a compile time check with no runtime overhead.
I am sure
one can solve this via template tricks^H^H^H technology, but I don't
have a
solution at hand. Not a blocker for the CL, but just something to keep
in mind. Done. http://codereview.chromium.org/9638014/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
