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?

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.

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?

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?

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?

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.

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.

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?

http://codereview.chromium.org/155344

--~--~---------~--~----~------------~-------~--~----~
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
-~----------~----~----~----~------~----~------~--~---

Reply via email to