Refactor type checks in v8natives.js and runtime.js....

Refactor type checks in v8natives.js and runtime.js.

This includes adding a new inline IsSpecObject method to the code
generator.  The old approach was somehow ineffecient since we would
call both IsObject, IsUndetectable and IsFunction to determine if
something was an object according to the spec. This change introduces
a new macro that determines if something is an object according to the
spec (and this does not include null). 

This change also corrects a few places where undetectable objects was
not allowed even when they should be (priorly they would use only
IS_SPEC_OBJECT_OR_NULL, which would return false on an undetectable
object, the new IS_SPEC_OBJECT returns true on an undetectable object.


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

[William Hesse, 2010-07-15 14:22:06]

Some comments to start with.

http://codereview.chromium.org/2877018/diff/1/2
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/2877018/diff/1/2#newcode4776
src/arm/codegen-arm.cc:4776: true_target()->Branch(eq);
This should be false_target, right?

http://codereview.chromium.org/2877018/diff/1/2#newcode4780
src/arm/codegen-arm.cc:4780: // If this is an undetectable object return true
immediately.
Check for undetectable seems wrong, since it would accept undetectable strings. 
Why do we even check this property at all?

http://codereview.chromium.org/2877018/diff/1/2#newcode4788
src/arm/codegen-arm.cc:4788: __ cmp(possible_object, Operand(JS_FUNCTION_TYPE));
We assume that JS_FUNCTION_TYPE is the last type throughout the code generator,
so this check can be omitted.

http://codereview.chromium.org/2877018/diff/1/12
File src/v8natives.js (left):

http://codereview.chromium.org/2877018/diff/1/12#oldcode228
src/v8natives.js:228: if (!IS_SPEC_OBJECT_OR_NULL(V) && !IS_UNDETECTABLE(V))
return false;
Is the original line correct?  Why could an undetectable non-object by a
prototype?

http://codereview.chromium.org/2877018/diff/1/12#oldcode236
src/v8natives.js:236: if (!IS_SPEC_OBJECT_OR_NULL(this)) return false;
This is all messed up.

[Rico, 2010-07-16 09:30:03]

Bill, could you have another look.

I simplified the IsSpecObject implementation in the code generators. It no
longer returns true on null and non-object undetectables.

http://codereview.chromium.org/2877018/diff/1/2
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/2877018/diff/1/2#newcode4776
src/arm/codegen-arm.cc:4776: true_target()->Branch(eq);
On 2010/07/15 14:22:06, William Hesse wrote:
> This should be false_target, right?
I changed this method eliminating this check

http://codereview.chromium.org/2877018/diff/1/2#newcode4780
src/arm/codegen-arm.cc:4780: // If this is an undetectable object return true
immediately.
On 2010/07/15 14:22:06, William Hesse wrote:
> Check for undetectable seems wrong, since it would accept undetectable
strings. 
> Why do we even check this property at all?

Good point, as stated above I removed a lot of checks from this method,
including this

http://codereview.chromium.org/2877018/diff/1/2#newcode4788
src/arm/codegen-arm.cc:4788: __ cmp(possible_object, Operand(JS_FUNCTION_TYPE));
On 2010/07/15 14:22:06, William Hesse wrote:
> We assume that JS_FUNCTION_TYPE is the last type throughout the code
generator,
> so this check can be omitted.

Done.

http://codereview.chromium.org/2877018/diff/1/12
File src/v8natives.js (left):

http://codereview.chromium.org/2877018/diff/1/12#oldcode228
src/v8natives.js:228: if (!IS_SPEC_OBJECT_OR_NULL(V) && !IS_UNDETECTABLE(V))
return false;
On 2010/07/15 14:22:06, William Hesse wrote:
> Is the original line correct?  Why could an undetectable non-object by a
> prototype?
That is correct, the new version (well, the corrected new version) takes this
info account since IsSpecObject no longer recognizes undetectable non-objects
(it simply discards if an object is undetectable or not)

http://codereview.chromium.org/2877018/diff/1/12#oldcode236
src/v8natives.js:236: if (!IS_SPEC_OBJECT_OR_NULL(this)) return false;
On 2010/07/15 14:22:06, William Hesse wrote:
> This is all messed up.
Yes, none of these checks are neccesary, I exchanged them with a ToObject
call(as specified in the spec).

[William Hesse, 2010-07-16 11:08:36]

LGTM, if comments are addressed.

http://codereview.chromium.org/2877018/diff/35001/1016
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/2877018/diff/35001/1016#newcode4767
src/arm/codegen-arm.cc:4767: // It includes undetectable objects (as oppose to
IsObject).
as opposed to
or
// It includes undetectable objects (IsObject does not).

http://codereview.chromium.org/2877018/diff/35001/1022
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/2877018/diff/35001/1022#newcode6456
src/ia32/codegen-ia32.cc:6456: temp.Unuse();
temp is not declared in this function.
You cannot use value.reg() as a temp register unless you spill it from the
frame.  Otherwise, it might be shared by another local variable.

http://codereview.chromium.org/2877018/diff/35001/1028
File src/runtime.js (right):

http://codereview.chromium.org/2877018/diff/35001/1028#newcode588
src/runtime.js:588: if (IS_SPEC_OBJECT(x)) return false;
Why not return !IS_SPEC_OBJECT(x); ?

http://codereview.chromium.org/2877018/diff/35001/1030
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/2877018/diff/35001/1030#newcode5739
src/x64/codegen-x64.cc:5739: __ CmpObjectType(value.reg(), FIRST_JS_OBJECT_TYPE,
value.reg);
kScratchRegister can be used here.

[Rico, 2010-07-16 11:19:46]

http://codereview.chromium.org/2877018/diff/35001/1016
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/2877018/diff/35001/1016#newcode4767
src/arm/codegen-arm.cc:4767: // It includes undetectable objects (as oppose to
IsObject).
On 2010/07/16 11:08:36, William Hesse wrote:
> as opposed to
> or
> // It includes undetectable objects (IsObject does not).
> 

Done.

http://codereview.chromium.org/2877018/diff/35001/1022
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/2877018/diff/35001/1022#newcode6456
src/ia32/codegen-ia32.cc:6456: temp.Unuse();
On 2010/07/16 11:08:36, William Hesse wrote:
> temp is not declared in this function.
> You cannot use value.reg() as a temp register unless you spill it from the
> frame.  Otherwise, it might be shared by another local variable.

Done.

http://codereview.chromium.org/2877018/diff/35001/1028
File src/runtime.js (right):

http://codereview.chromium.org/2877018/diff/35001/1028#newcode588
src/runtime.js:588: if (IS_SPEC_OBJECT(x)) return false;
On 2010/07/16 11:08:36, William Hesse wrote:
> Why not return !IS_SPEC_OBJECT(x); ?
> 

Done.

http://codereview.chromium.org/2877018/diff/35001/1030
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/2877018/diff/35001/1030#newcode5739
src/x64/codegen-x64.cc:5739: __ CmpObjectType(value.reg(), FIRST_JS_OBJECT_TYPE,
value.reg);
On 2010/07/16 11:08:36, William Hesse wrote:
> kScratchRegister can be used here.

Done.