Next round of comments. I think there is still room for simplifications but
we
are definitely getting closer.
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.
and receiver must be a heap object.
http://codereview.chromium.org/2801018/diff/10003/29008#newcode123
src/ia32/stub-cache-ia32.cc:123: const int bit_flags = (1 <<
Map::kHasNamedInterceptor) |
bit_flags -> kInterceptorOrAccessCheckNeededMask
or something similar.
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.
or requires access checks.
http://codereview.chromium.org/2801018/diff/10003/29008#newcode132
src/ia32/stub-cache-ia32.cc:132: if (check_object_type) {
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?
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.
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?
http://codereview.chromium.org/2801018/diff/10003/29008#newcode190
src/ia32/stub-cache-ia32.cc:190: ASSERT(!Heap::InNewSpace(name));
Not necessary. You have already asserted that name is a symbol.
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
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 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.
http://codereview.chromium.org/2801018/diff/10003/29008#newcode871
src/ia32/stub-cache-ia32.cc:871: if (!name->IsSymbol()) {
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?
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 &&
Please add a comment explaining this condition.
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) {
indentation
http://codereview.chromium.org/2801018/diff/10003/29010#newcode141
src/ic.cc:141: for (Object* i = receiver; i != end; i =
i->GetPrototype()) {
i -> current?
http://codereview.chromium.org/2801018/diff/10003/29010#newcode520
src/ic.cc:520: State state,
Indentation, not your fault, but could you fix it? :)
http://codereview.chromium.org/2801018/diff/10003/29010#newcode530
src/ic.cc:530: if (lookup->holder() != object->GetPrototype() &&
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
?
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.
This methods -> These methods
http://codereview.chromium.org/2801018/diff/10003/29015
File src/stub-cache.cc (right):
http://codereview.chromium.org/2801018/diff/10003/29015#newcode406
src/stub-cache.cc:406: PROFILE(CodeCreateEvent(Logger::STORE_IC_TAG,
Code::cast(code), name));
Good catch, thanks. :)
http://codereview.chromium.org/2801018/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
