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); On 2009/10/20 11:38:42, Christian Plesner Hansen wrote: > It's not obvious how the length will be interpreted. Is it the number of bytes > of data or the number of elements? Changed to "number_of_elements". 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, On 2009/10/20 13:35:08, Kevin Millikin wrote: > The args are kind of scrunched up. Maybe break right after "=". Done. 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. On 2009/10/20 11:38:42, Christian Plesner Hansen wrote: > 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? Filed issue 476 and changed TODOs. I think TODO(username) does pass the lint check, though. 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) \ On 2009/10/20 13:35:08, Kevin Millikin wrote: > It's annoying, but you should align the backslashes for the whole macro. Done. 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, On 2009/10/20 11:38:42, Christian Plesner Hansen wrote: > This parameters should be on the next line. Ditto NewPixelArray. Done. 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) { On 2009/10/20 11:38:42, Christian Plesner Hansen wrote: > Couldn't this be implemented using RootIndexForExternalArrayType? Good point. I didn't realize this since I implemented them in the opposite order. Done. http://codereview.chromium.org/293023/diff/1043/49#newcode1693 Line 1693: ASSERT(false && "Illegal external array type"); On 2009/10/20 13:35:08, Kevin Millikin wrote: > We usually use UNREACHABLE() here. Fixed here and throughout the code. 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) { On 2009/10/20 11:38:42, Christian Plesner Hansen wrote: > 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). The problem is there are a few different kinds of switch statements associated with these new types. For the time being I'd like to leave them as is, but am open to refactoring them later. http://codereview.chromium.org/293023/diff/1043/56#newcode286 Line 286: ASSERT(false); On 2009/10/20 13:35:08, Kevin Millikin wrote: > We usually use UNREACHABLE() here. Done. http://codereview.chromium.org/293023/diff/1043/56#newcode311 Line 311: ASSERT(false); On 2009/10/20 13:35:08, Kevin Millikin wrote: > Same. Done. http://codereview.chromium.org/293023/diff/1043/56#newcode875 Line 875: bool ic_set = false; On 2009/10/20 13:35:08, Kevin Millikin wrote: > 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); It turns out this doesn't work because there is no active HandleScope. Based on your suggestion, I've modified the code here and below to re-assign a Code pointer. 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() || On 2009/10/20 11:38:42, Christian Plesner Hansen wrote: > If all these types come together the enum you can use a range check instead. OK, made this change and added a comment in objects.h. http://codereview.chromium.org/293023/diff/1043/71#newcode2017 Line 2017: void ExternalArray::set_external_pointer(void* value, WriteBarrierMode mode) { On 2009/10/20 11:38:42, Christian Plesner Hansen wrote: > Do you need the 'mode' argument -- it isn't used. The method signature is defined by the DECL_ACCESSORS macro. http://codereview.chromium.org/293023/diff/1043/71#newcode2829 Line 2829: if (array->IsExternalArray()) { On 2009/10/20 11:38:42, Christian Plesner Hansen wrote: > A switch on the elements' instance type would be more efficient here. Done. http://codereview.chromium.org/293023/diff/1043/71#newcode2882 Line 2882: bool JSObject::HasExternalByteElements() { On 2009/10/20 11:38:42, Christian Plesner Hansen wrote: > Consider checking this using the elements' instance type. For parity with the other variants, I'd like to leave this as is. 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: { On 2009/10/20 13:35:08, Kevin Millikin wrote: > We usually leave off the braces unless needed by a variable declaration in the > case branch. Done. http://codereview.chromium.org/293023/diff/1043/51#newcode7306 Line 7306: uint32_t index, Object* value) { On 2009/10/20 11:38:42, Christian Plesner Hansen wrote: > Last param should be on next line. Done. 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 On 2009/10/20 11:38:42, Christian Plesner Hansen wrote: > 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. I don't think so. These arrays are implicitly 4 GB maximum in size, and this code is only intended to report errors which the WebGL spec requires but which can't really happen with normal JavaScript arrays. http://codereview.chromium.org/293023/diff/1043/58#newcode5310 Line 5310: uint32_t len = array->length(); On 2009/10/20 13:35:08, Kevin Millikin wrote: > We have Min in utils.h so you can write > uint32_t len = Min(range, array->length()); It turns out to be more gross than this due to a need for a static_cast<uint32_t>(array->length()), but done. http://codereview.chromium.org/293023/diff/1043/58#newcode5319 Line 5319: if (visitor != NULL) { On 2009/10/20 11:38:42, Christian Plesner Hansen wrote: > Can't this check be hoisted out of the loop. This also applies below. Yes, but it was necessary to be careful about returning the number of elements iterated. Done. http://codereview.chromium.org/293023/diff/1043/58#newcode5350 Line 5350: return num_of_elements; On 2009/10/20 13:35:08, Kevin Millikin wrote: > Isn't num_of_elements always len at the end, or am I missing something? Yes, it is. Changed along with hoisting out the NULL check on the visitor. http://codereview.chromium.org/293023/diff/1043/65 File src/x64/assembler-x64.cc (right): http://codereview.chromium.org/293023/diff/1043/65#newcode1258 Line 1258: void Assembler::movw(Register dst, const Operand& src) { On 2009/10/20 12:56:39, William Hesse wrote: > The operand-size prefix 0x66 is necessary. In this case, the top 48 bits will > be unmodified. You probably want movzx and movsx. *Thank you* for catching this error. The unit tests should have caught it, but the nature of the bug was that it would blow away the next array element. I've expanded the unit tests to contain downward loops, which catch the bug. I also removed movw(Register, const Operand&) as it was unused and as you point out it's useless since it doesn't change the high 48 bits. http://codereview.chromium.org/293023/diff/1043/65#newcode1270 Line 1270: emit(0x89); On 2009/10/20 12:56:39, William Hesse wrote: > One of these two should be 8B. Yes, thank you again. I've removed the variant with opcode 0x8B. http://codereview.chromium.org/293023/diff/1043/65#newcode1457 Line 1457: On 2009/10/20 12:56:39, William Hesse wrote: > The 16-bit version needs the operand-size prefix. Why isn't the 16-bit version > just movsxlq plus the prefix, based on opcode 0x63? As we just discussed, these are actually the correct encodings due to the semantics of these instructions; verified against the 32-bit port. http://codereview.chromium.org/293023/diff/1043/64 File src/x64/macro-assembler-x64.cc (right): http://codereview.chromium.org/293023/diff/1043/64#newcode587 Line 587: return zero; On 2009/10/20 12:56:39, William Hesse wrote: > Why not testq(src, src) > return no_sign; The high 32 bits of the 64-bit register are not set in this case. 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 On 2009/10/20 11:38:42, Christian Plesner Hansen wrote: > You can use SNPrintF from platform.h instead. Done. http://codereview.chromium.org/293023 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
