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. > 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__. > 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. > 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. > 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. > 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. Cheers, Kasper --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
