Refactoring only: Make the handling of PropertyType more explicit.

Refactoring only: Make the handling of PropertyType more explicit.

Do not rely on 'default' clauses or 'if's when analysing a PropertyType, because
this makes it hard to find the relevant places when a new type is added. Note
that the detection of "phantom property types" is left untouched, because this
might have a performance impact, especially for the GC (to be investigated).

This is a preliminary step for introducing a new kind of map transition.

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

[Kevin Millikin (Chromium), 2011-11-07 14:13:39]

I don't immediately see why some of the types are unreachable in
KeyedStoreIC::UpdateCaches.

http://codereview.chromium.org/8491016/diff/1/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8491016/diff/1/src/ic.cc#newcode1396
src/ic.cc:1396: case HANDLER:
Is HANDLER unreachable because the caller takes care of proxies?  In that case,
then !receiver->IsJSProxy() is also one of the preconditions and should be
asserted on entry to this function.

I find it much more clear to get a precondition assertion violation on entry
rather than as a switch match failure in the middle of the function.

http://codereview.chromium.org/8491016/diff/1/src/ic.cc#newcode1727
src/ic.cc:1727: case CONSTANT_FUNCTION:
I guess we should not get NULL_DESCRIPTOR here, and HANDLER is covered because
the caller ensures it (?).

I don't immediately see why CONSTANT_FUNCTION, INTERCEPTOR, and
ELEMENTS_TRANSITION is impossible.  It definitely needs a short comment.

http://codereview.chromium.org/8491016/diff/1/src/v8globals.h
File src/v8globals.h (right):

http://codereview.chromium.org/8491016/diff/1/src/v8globals.h#newcode351
src/v8globals.h:351: inline bool IsTransitionType(PropertyType type) {
I'm a bit nervous about this.  I honestly don't know if compilers do a good job
of inlining the switch.  I wonder if it's best just to put this in property.cc
(and then you don't need checks.h in this header)?

I'm also nervous about the near-circularity of putting checks.h into
v8globals.h.  v8.h contains the headers that need a fixed include order for one
reason or another, and it has v8globals.h followed by v8checks.h (which includes
checks.h).

So, not exactly a circularity but already getting tangled.

[Sven Panne, 2011-11-08 07:36:09]

I've addressed the "impossible cases" parts of the review, the misplacement of
PropertyType and its related predicate will be tackled in a separate patch for
this CL.

PTAL

http://codereview.chromium.org/8491016/diff/1/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8491016/diff/1/src/ic.cc#newcode1396
src/ic.cc:1396: case HANDLER:
On 2011/11/07 14:13:39, Kevin Millikin wrote:
> Is HANDLER unreachable because the caller takes care of proxies?  In that
case,
> then !receiver->IsJSProxy() is also one of the preconditions and should be
> asserted on entry to this function.
> 
> I find it much more clear to get a precondition assertion violation on entry
> rather than as a switch match failure in the middle of the function.

Done.

http://codereview.chromium.org/8491016/diff/1/src/ic.cc#newcode1727
src/ic.cc:1727: case CONSTANT_FUNCTION:
On 2011/11/07 14:13:39, Kevin Millikin wrote:
> I guess we should not get NULL_DESCRIPTOR here, and HANDLER is covered because
> the caller ensures it (?).
> 
> I don't immediately see why CONSTANT_FUNCTION, INTERCEPTOR, and
> ELEMENTS_TRANSITION is impossible.  It definitely needs a short comment.

Done.

http://codereview.chromium.org/8491016/diff/1/src/v8globals.h
File src/v8globals.h (right):

http://codereview.chromium.org/8491016/diff/1/src/v8globals.h#newcode351
src/v8globals.h:351: inline bool IsTransitionType(PropertyType type) {
On 2011/11/07 14:13:39, Kevin Millikin wrote:
> I'm a bit nervous about this.  I honestly don't know if compilers do a good
job
> of inlining the switch.  I wonder if it's best just to put this in property.cc
> (and then you don't need checks.h in this header)?
> 
> I'm also nervous about the near-circularity of putting checks.h into
> v8globals.h.  v8.h contains the headers that need a fixed include order for
one
> reason or another, and it has v8globals.h followed by v8checks.h (which
includes
> checks.h).
> 
> So, not exactly a circularity but already getting tangled.

I'm looking into this, but this is a bit unrelated to the other changes, so I'll
tackle this in another patch to the CL to make the review easier.

[Kevin Millikin (Chromium), 2011-11-08 08:26:23]

LGTM.