http://codereview.chromium.org/2801018/diff/10003/29008 File src/ia32/stub-cache-ia32.cc (right):
http://codereview.chromium.org/2801018/diff/10003/29008#newcode108 src/ia32/stub-cache-ia32.cc:108: // Name must be a symbol. On 2010/06/30 12:10:16, Mads Ager wrote:
and receiver must be a heap object.
Done. http://codereview.chromium.org/2801018/diff/10003/29008#newcode123 src/ia32/stub-cache-ia32.cc:123: const int bit_flags = (1 << Map::kHasNamedInterceptor) | On 2010/06/30 12:10:16, Mads Ager wrote:
bit_flags -> kInterceptorOrAccessCheckNeededMask
or something similar.
Done. http://codereview.chromium.org/2801018/diff/10003/29008#newcode125 src/ia32/stub-cache-ia32.cc:125: // Bail out if the receiver has a named interceptor. On 2010/06/30 12:10:16, Mads Ager wrote:
or requires access checks.
Done. http://codereview.chromium.org/2801018/diff/10003/29008#newcode132 src/ia32/stub-cache-ia32.cc:132: if (check_object_type) { On 2010/06/30 12:10:16, Mads Ager wrote:
I don't understand this part. You have just checked that r0 is
JS_OBJECT_TYPE.
If it is not, you bail out. Now you are doing another conditional
instance type
check. Which of them do you want?
Removed. The first object in the chain could be not-null and not-js, but I didn't spotted that the checking code is the same. http://codereview.chromium.org/2801018/diff/10003/29008#newcode178 src/ia32/stub-cache-ia32.cc:178: // Having undefined at thins place means the name is not contained. On 2010/06/30 12:10:16, Mads Ager wrote:
thins -> this
You should be more explicit in this comment. I now understand why you
don't have
to explictly check for deleted elements here because we use null for
delete
elements instead of undefined, so check for undefined you are checking
both for
existing and for deleted elements. Could you expand the comment to be
explicit
about that?
Done. http://codereview.chromium.org/2801018/diff/10003/29008#newcode190 src/ia32/stub-cache-ia32.cc:190: ASSERT(!Heap::InNewSpace(name)); On 2010/06/30 12:10:16, Mads Ager wrote:
Not necessary. You have already asserted that name is a symbol.
Done. http://codereview.chromium.org/2801018/diff/10003/29008#newcode845 src/ia32/stub-cache-ia32.cc:845: // Make sure there's no overlap between scratch and the other On 2010/06/30 12:10:16, Mads Ager wrote:
I think this can be simplified because we only allow the receiver to
be a
dictionary case object. I think we can keep this code as it was except
that we
check for slow properties on the object first. If the object has slow
properties
we call <div style="display: inline; height: auto; position: absolute;
visibility: hidden; width: auto; ">GenerateNegativeDictionaryLookup</div>GenerateNegativeDictionaryLookup and use CheckMaps with
object->GetPrototype().
I would like to get this in as a minimal delta to what is there
already so it is
easy to follow.
For the rest of the chain we can assert that they have fast
properties. You actually suggest to move a piece of code out of the loop. I don't think it's simplification. It would make the code less general giving nothing back (what sense to propagate the decision to not allow slow objects in the middle through the codebase?) If the point is to reduce the delta I would suggest restore the CheckMaps function, add the 'name' paramater and move the new code there (together with GenerateNegativeLookup). However I dislike it because: 1) CheckMaps is used only once 2) CheckPrototype represent almost the same level of abstraction 3) it multiplies entities unreasonably. http://codereview.chromium.org/2801018/diff/10003/29008#newcode871 src/ia32/stub-cache-ia32.cc:871: if (!name->IsSymbol()) { On 2010/06/30 12:10:16, Mads Ager wrote:
When do we get here where name is not a symbol? Could this be an
assert that
name is a symbol? For normal call ICs the name will be a symbol. For
keyed call
ICs I would think that a symbol check takes place before the prototype
check?
Can we make this an assert that they name is a symbol?
CheckPrototypes is definitely called with non-symbol value (I added such an assert at the begin of the method and it broke a lot of tests). That calls do not cause negative lookups but I think it's much safer to be ready for this case. http://codereview.chromium.org/2801018/diff/10003/29009 File src/ic-inl.h (right): http://codereview.chromium.org/2801018/diff/10003/29009#newcode96 src/ic-inl.h:96: if (holder != object && On 2010/06/30 12:10:16, Mads Ager wrote:
Please add a comment explaining this condition.
Done. http://codereview.chromium.org/2801018/diff/10003/29010 File src/ic.cc (right): http://codereview.chromium.org/2801018/diff/10003/29010#newcode139 src/ic.cc:139: Object* receiver) { On 2010/06/30 12:10:16, Mads Ager wrote:
indentation
Done. http://codereview.chromium.org/2801018/diff/10003/29010#newcode141 src/ic.cc:141: for (Object* i = receiver; i != end; i = i->GetPrototype()) { On 2010/06/30 12:10:16, Mads Ager wrote:
i -> current?
Done. http://codereview.chromium.org/2801018/diff/10003/29010#newcode520 src/ic.cc:520: State state, On 2010/06/30 12:10:16, Mads Ager wrote:
Indentation, not your fault, but could you fix it? :)
Done. http://codereview.chromium.org/2801018/diff/10003/29010#newcode530 src/ic.cc:530: if (lookup->holder() != object->GetPrototype() && On 2010/06/30 12:10:16, Mads Ager wrote:
I'm not sure this condition is the right one? If the holder is not the
receiver
we should check that there are no normal objects starting from the
prototype.
The current check would disallow stuff like:
normal receiver -proto-> fast properties -proto-> function here on
fast
properties object
?
Done. http://codereview.chromium.org/2801018/diff/10003/29011 File src/ic.h (right): http://codereview.chromium.org/2801018/diff/10003/29011#newcode121 src/ic.h:121: // This methods should not be called with undefined or null. On 2010/06/30 12:10:16, Mads Ager wrote:
This methods -> These methods
Done. http://codereview.chromium.org/2801018/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
