Uploaded a new version, ready for the next round. -Ivan
http://codereview.chromium.org/159263/diff/1012/1022 File src/api.h (right): http://codereview.chromium.org/159263/diff/1012/1022#newcode102 Line 102: ASSERT(value()->GetElementsKind() == On 2009/07/27 07:43:41, Kasper Lund wrote: > Why not keep the HasFastElements checker? Part of my approach was to remove HasFastElement from the JSObject interface to make sure that I caught all uses of it. Apparently I did not put some back... http://codereview.chromium.org/159263/diff/1012/1018 File src/handles.cc (right): http://codereview.chromium.org/159263/diff/1012/1018#newcode349 Line 349: *value.location() = *number; // Overwrite the value being passed in. On 2009/07/27 07:43:41, Kasper Lund wrote: > This seems fishy to me. Why not just do 'value = number'? Would have been too easy. Done. http://codereview.chromium.org/159263/diff/1012/1017 File src/heap.cc (right): http://codereview.chromium.org/159263/diff/1012/1017#newcode1874 Line 1874: size > MaxObjectSizeInPagedSpace() ? LO_SPACE : OLD_DATA_SPACE; On 2009/07/27 07:43:41, Kasper Lund wrote: > Do we ever get pixel arrays in LO_SPACE? Aren't they super small? Maybe change > to an assert? Done. Initially pixel arrays contained the data instead of an external pointer. At that point they would grow pretty quickly. I missed this change here, thanks for pointing it out. http://codereview.chromium.org/159263/diff/1012/1017#newcode1889 Line 1889: AllocationSpace space = On 2009/07/27 07:43:41, Kasper Lund wrote: > Do we ever get pixel arrays in LO_SPACE? Aren't they super small? Maybe change > to an assert? Done. This method has been removed. http://codereview.chromium.org/159263/diff/1012/1017#newcode2062 Line 2062: ASSERT(JSObject::cast(result)->GetElementsKind() == JSObject::FAST_ELEMENTS); On 2009/07/27 07:43:41, Kasper Lund wrote: > I would keep HasFastElements. Done. http://codereview.chromium.org/159263/diff/1012/1029 File src/heap.h (right): http://codereview.chromium.org/159263/diff/1012/1029#newcode434 Line 434: static Object* AllocatePixelArray(int lenght, uint8_t* external_pointer); On 2009/07/27 07:43:41, Kasper Lund wrote: > Do we really need this variant? Is it because it's faster than having a default > value for pretenure? The only reason is because I modeled it after the ByteArray code. The redundant definition is gone. http://codereview.chromium.org/159263/diff/1012/1021 File src/ia32/ic-ia32.cc (right): http://codereview.chromium.org/159263/diff/1012/1021#newcode294 Line 294: __ cmp(eax, FieldOperand(ecx, FixedArray::kLengthOffset)); On 2009/07/27 07:43:41, Kasper Lund wrote: > Shouldn't this be PixelArray::kLengthOffset? Done. http://codereview.chromium.org/159263/diff/1012/1021#newcode422 Line 422: __ cmp(ebx, FieldOperand(ecx, FixedArray::kLengthOffset)); On 2009/07/27 07:43:41, Kasper Lund wrote: > PixelArray::kLengthOffset? Done. http://codereview.chromium.org/159263/diff/1012/1019 File src/objects.cc (right): http://codereview.chromium.org/159263/diff/1012/1019#newcode1153 Line 1153: break; On 2009/07/27 07:43:41, Kasper Lund wrote: > Remove this break? Done. http://codereview.chromium.org/159263/diff/1012/1019#newcode2249 Line 2249: CHECK(!HasPixelElements()); On 2009/07/27 07:43:41, Kasper Lund wrote: > Maybe change to an ASSERT? Done. http://codereview.chromium.org/159263/diff/1012/1019#newcode2419 Line 2419: // TODO(iposva): Cleanup. On 2009/07/27 07:43:41, Kasper Lund wrote: > What kind of cleanup is missing here? Done. Needed to check whether this code could be reached. It can. http://codereview.chromium.org/159263/diff/1012/1019#newcode2762 Line 2762: // TODO(iposva): Cleanup On 2009/07/27 07:43:41, Kasper Lund wrote: > ? Done. Covered by the assert. http://codereview.chromium.org/159263/diff/1012/1019#newcode2884 Line 2884: JSObject* jsObject = JSObject::cast(obj); On 2009/07/27 07:43:41, Kasper Lund wrote: > Weird naming here? jsObject -> js_object (not your fault, but anyway) Done. http://codereview.chromium.org/159263/diff/1012/1019#newcode3110 Line 3110: UNIMPLEMENTED(); On 2009/07/27 07:43:41, Kasper Lund wrote: > I think you have to deal with this and add a test case that proves that the code > works. I don't think JSArray can have its elements set to contain a pixel array. I added an ASSERT to that extent at the beginning of the function. http://codereview.chromium.org/159263/diff/1012/1019#newcode5323 Line 5323: break; On 2009/07/27 07:43:41, Kasper Lund wrote: > Having a break after return seems a bit weird. Done. Old habit... http://codereview.chromium.org/159263/diff/1012/1024 File src/objects.h (right): http://codereview.chromium.org/159263/diff/1012/1024#newcode2460 Line 2460: // In particular write access do clamp the values to 0 or 255 if the value On 2009/07/27 07:43:41, Kasper Lund wrote: > do clamp -> clamps Done. http://codereview.chromium.org/159263/diff/1012/1024#newcode2472 Line 2472: // This accessor does apply the correct conversion and clamping on the value. On 2009/07/27 07:43:41, Kasper Lund wrote: > does apply -> applies Done. http://codereview.chromium.org/159263/diff/1012/1023 File src/runtime.cc (right): http://codereview.chromium.org/159263/diff/1012/1023#newcode174 Line 174: UNIMPLEMENTED(); On 2009/07/27 07:43:41, Kasper Lund wrote: > Implementation? Actually since pixel arrays cannot be created using an object literal I added an ASSERT(!copy->HasPixelElements()) before the switch statement. http://codereview.chromium.org/159263/diff/1012/1023#newcode3733 Line 3733: if (array->GetElementsKind() != JSObject::FAST_ELEMENTS) { On 2009/07/27 07:43:41, Kasper Lund wrote: > Keep as !HasFastElements. Done. http://codereview.chromium.org/159263/diff/1012/1023#newcode5209 Line 5209: UNIMPLEMENTED(); On 2009/07/27 07:43:41, Kasper Lund wrote: > This probably needs an implementation too. It seems to be used by the debugger. Done. One needs to do amazing __proto__ gymnastics to get here though. http://codereview.chromium.org/159263/diff/1012/1013 File test/cctest/test-api.cc (right): http://codereview.chromium.org/159263/diff/1012/1013#newcode7585 Line 7585: "};" On 2009/07/27 07:43:41, Kasper Lund wrote: > Why ;? (repeated) Done. http://codereview.chromium.org/159263/diff/1012/1013#newcode7633 Line 7633: result = CompileRun("pixels[7] = undefined;" On 2009/07/27 07:43:41, Kasper Lund wrote: > I might be nice to make sure this also works from an IC (add loop). Done. http://codereview.chromium.org/159263/diff/1012/1013#newcode7638 Line 7638: result = CompileRun("pixels[6] = '2.3';" On 2009/07/27 07:43:41, Kasper Lund wrote: > I might be nice to make sure this also works from an IC (add loop). Done. http://codereview.chromium.org/159263 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
