Some style comments. I think the approach looks OK. I'm eager to see the IC code.
LGTM. http://codereview.chromium.org/293023/diff/1043/48 File src/api.cc (right): http://codereview.chromium.org/293023/diff/1043/48#newcode2327 Line 2327: i::Handle<i::ExternalArray> array = i::Factory::NewExternalArray(length, The args are kind of scrunched up. Maybe break right after "=". http://codereview.chromium.org/293023/diff/1043/74 File src/builtins.h (right): http://codereview.chromium.org/293023/diff/1043/74#newcode77 Line 77: V(KeyedLoadIC_ExternalByteArray, KEYED_LOAD_IC, MEGAMORPHIC) \ It's annoying, but you should align the backslashes for the whole macro. http://codereview.chromium.org/293023/diff/1043/49 File src/heap.cc (right): http://codereview.chromium.org/293023/diff/1043/49#newcode1693 Line 1693: ASSERT(false && "Illegal external array type"); We usually use UNREACHABLE() here. http://codereview.chromium.org/293023/diff/1043/55 File src/ia32/ic-ia32.cc (right): http://codereview.chromium.org/293023/diff/1043/55#newcode406 Line 406: Generate(masm, ExternalReference(Runtime::kSetProperty)); Thank you. http://codereview.chromium.org/293023/diff/1043/56 File src/ic.cc (right): http://codereview.chromium.org/293023/diff/1043/56#newcode286 Line 286: ASSERT(false); We usually use UNREACHABLE() here. http://codereview.chromium.org/293023/diff/1043/56#newcode311 Line 311: ASSERT(false); Same. http://codereview.chromium.org/293023/diff/1043/56#newcode875 Line 875: bool ic_set = false; This is OK, but you could write: Handle<Code> stub = Handle<Code>(generic_stub()); if (object->IsJSObject()) { ... if (receiver->HasExternalArrayElements()) { stub = Handle<Code>(external_array_stub(receiver->GetElementsKind())); } } set_target(*stub); http://codereview.chromium.org/293023/diff/1043/51 File src/objects.cc (right): http://codereview.chromium.org/293023/diff/1043/51#newcode2458 Line 2458: case EXTERNAL_FLOAT_ELEMENTS: { We usually leave off the braces unless needed by a variable declaration in the case branch. http://codereview.chromium.org/293023/diff/1043/58 File src/runtime.cc (right): http://codereview.chromium.org/293023/diff/1043/58#newcode5310 Line 5310: uint32_t len = array->length(); We have Min in utils.h so you can write uint32_t len = Min(range, array->length()); http://codereview.chromium.org/293023/diff/1043/58#newcode5350 Line 5350: return num_of_elements; Isn't num_of_elements always len at the end, or am I missing something? http://codereview.chromium.org/293023 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
