Whew, big one. Overall lgtm, with comments.
http://codereview.chromium.org/293023/diff/1043/47 File include/v8.h (right): http://codereview.chromium.org/293023/diff/1043/47#newcode1300 Line 1300: int length); It's not obvious how the length will be interpreted. Is it the number of bytes of data or the number of elements? http://codereview.chromium.org/293023/diff/1043/72 File src/arm/ic-arm.cc (right): http://codereview.chromium.org/293023/diff/1043/72#newcode620 Line 620: // TODO(kbr): port specialized code. Unless this will be removed by the next changelist todos should be of the form TODO(N) where N is an issue tracker entry. Does this lint? http://codereview.chromium.org/293023/diff/1043/61 File src/factory.h (right): http://codereview.chromium.org/293023/diff/1043/61#newcode162 Line 162: static Handle<ExternalArray> NewExternalArray(int length, This parameters should be on the next line. Ditto NewPixelArray. http://codereview.chromium.org/293023/diff/1043/49 File src/heap.cc (right): http://codereview.chromium.org/293023/diff/1043/49#newcode1677 Line 1677: switch (array_type) { Couldn't this be implemented using RootIndexForExternalArrayType? http://codereview.chromium.org/293023/diff/1043/56 File src/ic.cc (right): http://codereview.chromium.org/293023/diff/1043/56#newcode269 Line 269: switch (elements_kind) { This dispatch pattern is used a lot. Maybe use a macro to enumerate the different types of external arrays like we do for other cases in objects.h and use it to generate all these dispatches (and possibly map allocation too). http://codereview.chromium.org/293023/diff/1043/71 File src/objects-inl.h (right): http://codereview.chromium.org/293023/diff/1043/71#newcode364 Line 364: return (IsExternalByteArray() || If all these types come together the enum you can use a range check instead. http://codereview.chromium.org/293023/diff/1043/71#newcode2017 Line 2017: void ExternalArray::set_external_pointer(void* value, WriteBarrierMode mode) { Do you need the 'mode' argument -- it isn't used. http://codereview.chromium.org/293023/diff/1043/71#newcode2829 Line 2829: if (array->IsExternalArray()) { A switch on the elements' instance type would be more efficient here. http://codereview.chromium.org/293023/diff/1043/71#newcode2882 Line 2882: bool JSObject::HasExternalByteElements() { Consider checking this using the elements' instance type. http://codereview.chromium.org/293023/diff/1043/51 File src/objects.cc (right): http://codereview.chromium.org/293023/diff/1043/51#newcode7306 Line 7306: uint32_t index, Object* value) { Last param should be on next line. http://codereview.chromium.org/293023/diff/1043/58 File src/runtime.cc (right): http://codereview.chromium.org/293023/diff/1043/58#newcode2583 Line 2583: // If the target object is a JSObject and has an ExternalArray as Do we need to deal with heap numbers here too? If we do we can't assume that an index fits into an uint32_t for NewIndexError. http://codereview.chromium.org/293023/diff/1043/58#newcode5319 Line 5319: if (visitor != NULL) { Can't this check be hoisted out of the loop. This also applies below. http://codereview.chromium.org/293023/diff/1043/46 File test/cctest/test-api.cc (right): http://codereview.chromium.org/293023/diff/1043/46#newcode41 Line 41: #define snprintf _snprintf You can use SNPrintF from platform.h instead. http://codereview.chromium.org/293023 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
