Thanks Søren. Nice catch with the null descriptors!
http://codereview.chromium.org/647015/diff/1/6 File src/bootstrapper.cc (right): http://codereview.chromium.org/647015/diff/1/6#newcode1326 src/bootstrapper.cc:1326: On 2010/02/18 14:07:41, Søren Gjesse wrote:
Not related to your change, but it looks as if this function works
differently
if from is in fast case. If from is in fast the to is only checked for
existing
property if we are looking at transfering an accessor, whereas if from
is in
slow case we check to for the existence of the property for all
properties. That is indeed weird. Let me look at that separately to not put too much into this change. http://codereview.chromium.org/647015/diff/1/4 File src/objects.cc (right): http://codereview.chromium.org/647015/diff/1/4#newcode1464 src/objects.cc:1464: if (result.IsPropertyOrTransition()) { On 2010/02/18 14:07:41, Søren Gjesse wrote:
I am not sure about NULL_DESCRIPTOR here? SetProperty has code to
"convert" a
NULL_DESCRIPTOR to a field the same way it does for
CONSTANT_TRANSITION. I guess
AddProperty will will end up overwriting the NULL_DESCRIPTOR with a MAP_TRANSITION or CONSTANT_TRANSITION.
You are absolutely right, we should allow NULL_DESCRIPTORS here - which means that we need to allow them i LocalLookupRealNamedProperty as well. We need to use IsFound here and add a comment. http://codereview.chromium.org/647015/diff/1/4#newcode1762 src/objects.cc:1762: if (result->IsPropertyOrTransition()) { On 2010/02/18 14:07:41, Søren Gjesse wrote:
Why do we want transitions here?
Because it is used by set as well as by get and by "real" we just mean that it is not an interceptor... This really should be IsFound() with a comment that we allow transitions, null descriptors and properties because this method is also used when setting properties. http://codereview.chromium.org/647015/diff/1/4#newcode1968 src/objects.cc:1968: // ADDED TO CLONE On 2010/02/18 14:07:41, Søren Gjesse wrote:
Please silence this comment :-).
Done. http://codereview.chromium.org/647015/diff/1/11 File src/property.h (right): http://codereview.chromium.org/647015/diff/1/11#newcode213 src/property.h:213: bool IsTransitionType() { On 2010/02/18 14:07:41, Søren Gjesse wrote:
IsTransitionType -> IsTransition or maybe just remove it, as I think
you have
removed the only use of it.
Done. http://codereview.chromium.org/647015 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
