Remove the LookupResult IsValid method because it is confusing....

Remove the LookupResult IsValid method because it is confusing.

Replaced IsValid by IsPropertyOrTransition and used IsProperty in most
of the places where IsValid was used before.  Most of the time when
inspecting a lookup result we really want to know if there is a real
property present.  Only for stores are we interested in transitions.

BUG=http://crbug.com/20104
TEST=cctest/test-api/NamedInterceptorMapTransitionRead

Committed: http://code.google.com/p/v8/source/detail?r=3901

[Søren Thygesen Gjesse, 2010-02-18 14:07:41]

LGTM

Nice cleanup!

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: 
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.

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()) {
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.

http://codereview.chromium.org/647015/diff/1/4#newcode1762
src/objects.cc:1762: if (result->IsPropertyOrTransition()) {
Why do we want transitions here?

http://codereview.chromium.org/647015/diff/1/4#newcode1968
src/objects.cc:1968: // ADDED TO CLONE
Please silence this comment :-).

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() {
IsTransitionType -> IsTransition or maybe just remove it, as I think you have
removed the only use of it.

[Mads Ager (chromium), 2010-02-18 15:05:24]

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.

[dglazkov, 2010-02-18 15:38:44]

Awesome! Thanks, guys!