LGTM with nits. Hairy stuff... o_O
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#newcode610 src/elements.cc:610: static MaybeObject* CopyElementsImpl(FixedArrayBase* from, 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#newcode989 src/elements.cc:989: return from; default: Layout... 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; 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... ;-) http://codereview.chromium.org/9638014/diff/3005/src/elements.h#newcode145 src/elements.h:145: void CopyObjectToObjectElements(AssertNoAllocation* no_gc, 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. http://codereview.chromium.org/9638014/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
