LGTM with comments.
https://chromiumcodereview.appspot.com/9638014/diff/3005/src/elements.cc File src/elements.cc (right): https://chromiumcodereview.appspot.com/9638014/diff/3005/src/elements.cc#newcode65 src/elements.cc:65: // identical. Note that the order must match that of the ElementsKind enum for 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. https://chromiumcodereview.appspot.com/9638014/diff/3005/src/elements.cc#newcode148 src/elements.cc:148: if (copy_size == -1) copy_size = from_obj->length() - from_start; 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. https://chromiumcodereview.appspot.com/9638014/diff/3005/src/elements.cc#newcode209 src/elements.cc:209: if (copy_size == -1) copy_size = from_obj->length() - from_start; I'd move this up too (see my other comment above). https://chromiumcodereview.appspot.com/9638014/diff/3005/src/elements.cc#newcode228 src/elements.cc:228: CHECK(maybe_value_object->ToObject(&value)); 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. https://chromiumcodereview.appspot.com/9638014/diff/3005/src/elements.cc#newcode245 src/elements.cc:245: if (copy_size == -1) copy_size = from_obj->length() - from_start; And once more, I'd move this up. https://chromiumcodereview.appspot.com/9638014/diff/3005/src/elements.cc#newcode695 src/elements.cc:695: to_start, copy_size); nit: indentation https://chromiumcodereview.appspot.com/9638014/diff/3005/src/elements.cc#newcode989 src/elements.cc:989: return from; default: nit: new line for "default:" please. https://chromiumcodereview.appspot.com/9638014/diff/3005/src/elements.cc#newcode1052 src/elements.cc:1052: NonStrictArgumentsElementsAccessor, very nitty nit: 4 spaces indent, not 5 :-) https://chromiumcodereview.appspot.com/9638014/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
