On Fri, Jul 10, 2009 at 1:35 PM, Kasper Lund<[email protected]> wrote: > Thanks a lot for the review. I'm trying out the change in the browser > and getting ready for submitting it. > > On Fri, Jul 10, 2009 at 11:28 AM, <[email protected]> wrote: >> Kasper, LGTM but I am not qualified enough to understand every bit. >> >> I asked you many questions in review---they are mostly for my education, >> so if you feel it slows us down, feel free to ignore. >> >> >> http://codereview.chromium.org/155344/diff/1/8 >> File src/arm/stub-cache-arm.cc (right): >> >> http://codereview.chromium.org/155344/diff/1/8#newcode361 >> Line 361: Register StubCompiler::CheckPrototypes(JSObject* object, >> maybe it should go to macro-assembler? > > My plan is to go the other way, and move the CheckMaps functionality > into the StubCompiler.
I see. > >> http://codereview.chromium.org/155344/diff/1/8#newcode375 >> Line 375: if (object->IsGlobalObject()) { >> just curious, I guess there might be only single global object in >> prototype chain (or no)? If yes, may be could break out of the loop >> after global object is met. > > There can be more than one if you assign to __proto__. Tnx. >> http://codereview.chromium.org/155344/diff/1/8#newcode382 >> Line 382: JSGlobalPropertyCell* cell = >> JSGlobalPropertyCell::cast(probe); >> for my education: why it must be the hole? > > Only deleted cells contain the hole value. Is it possible for this property be not a deleted one? Put in other words, is it possible to trigger else part of EnusurePropertyCell? Or even in this case we would get the hole? > >> http://codereview.chromium.org/155344/diff/1/8#newcode384 >> Line 384: __ mov(scratch, Operand(Handle<Object>(cell))); >> cell always live not in new space, correct? > > Correct. They live in their own old space. > >> http://codereview.chromium.org/155344/diff/1/8#newcode390 >> Line 390: object = JSObject::cast(object->GetPrototype()); >> just for my education: it's always safe to assume that prototype is >> JSObject if we started with JSObject? > > Yes and no. The prototype of an object can be null, which terminates > the prototype chain. Oh yes, null. Tnx for explanations. > >> http://codereview.chromium.org/155344/diff/1/8#newcode398 >> Line 398: void StubCompiler::GenerateLoadField(JSObject* object, >> cosmetic issue: is the move of methods needed---if you can persuade >> codereview to compare former and new versions of methods side by side, >> it might be easier to understand the change. By no means insisting. > > The tricky part is the definition of __ which changes in middle of the > stub-cache-<arch>.cc file. So the position in the file matters. BTW, I was long curious why this definition changes? > >> http://codereview.chromium.org/155344/diff/1/7 >> File src/objects-inl.h (right): >> >> http://codereview.chromium.org/155344/diff/1/7#newcode2656 >> Line 2656: ASSERT(!key->IsString() || details.IsDeleted() || >> details.index() > 0); >> why this change? just curious. > > It's now okay to create properties that are deleted. > >> http://codereview.chromium.org/155344/diff/1/2 >> File src/objects.cc (right): >> >> http://codereview.chromium.org/155344/diff/1/2#newcode1731 >> Line 1731: if (!IsGlobalObject()) result->DisallowCaching(); >> maybe move that up, as else clause to if at 1715? > > That doesn't work, because 1715 is covered by another if that checks > if the property exists at all. You're right, sorry. yours, anton. --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
