Add Object.create from ECMAScript5. Supports value, writable, enumerable, ge...

Add Object.create from ECMAScript5.  Supports value, writable, enumerable, get
and set.  Doesn't support configurable yet.  See
http://code.google.com/p/v8/issues/detail?id=460

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

[Christian Plesner Hansen, 2009-12-08 04:30:52]

Lots of comments.  Needs a second iteration.

http://codereview.chromium.org/463040/diff/5/1006
File src/messages.js (right):

http://codereview.chromium.org/463040/diff/5/1006#newcode162
src/messages.js:162: value_and_getter_setter:      "Invalid property.  'value'
present on property with getter or setter",
This sounds confusing.  How about something along the lines of "A property
cannot have both a value and accessors".

http://codereview.chromium.org/463040/diff/5/1006#newcode163
src/messages.js:163: proto_object_or_null:         'Object prototype may only be
an Object or null',
Inconsistent string delimiters.

http://codereview.chromium.org/463040/diff/5/1005
File src/v8natives.js (right):

http://codereview.chromium.org/463040/diff/5/1005#newcode286
src/v8natives.js:286: if (desc === void 0) return false;
If you're deliberately avoiding IS_UNDEFINED please document why.  Otherwise you
should use IS_UNDEFINED.

http://codereview.chromium.org/463040/diff/5/1005#newcode312
src/v8natives.js:312: throw MakeTypeError(property_desc_object, [obj]);
You mean "property_desc_object" right, with quotes?

http://codereview.chromium.org/463040/diff/5/1005#newcode316
src/v8natives.js:316: var enumerable = obj.enumerable;
This doesn't seem to match what's in the spec.  Shouldn't it be something like

var enum = false;
if ('enumerable' in obj) {
  enum = ToBoolean(obj.enumerable);
}

Ditto, with variations, for every block below.  In any case |enumerable in obj|
certainly seems incorrect, it should be |"enumerable" in obj|.

http://codereview.chromium.org/463040/diff/5/1005#newcode336
src/v8natives.js:336: if (get !== void 0 && typeof(get) !== 'function') {
Use IS_FUNCTION.  Applies throughout.

http://codereview.chromium.org/463040/diff/5/1005#newcode360
src/v8natives.js:360: this.value_ = void 0;
The spec says nothing about these properties being present on property
descriptors.  This is both a problem in and of itself but it also allows people
to put a property descriptor into an inconsistent state.  Also, I can't find the
style guide right now but I believe variables should be lowerCamelCase.  This
applies throughout.

http://codereview.chromium.org/463040/diff/5/1005#newcode373
src/v8natives.js:373: PropertyDescriptor.prototype.set_value = function(value) {
Naming convention should be lowerCamelCase.

http://codereview.chromium.org/463040/diff/5/1005#newcode415
src/v8natives.js:415: if (typeof(desc.get_) === 'function') %DefineAccessor(obj,
p, GETTER, desc.get_, flag);
Use IS_FUNCTION.

http://codereview.chromium.org/463040/diff/5/1005#newcode440
src/v8natives.js:440: for (var key in props) {
This will include properties in the prototype chain of 'properties', only own
properties should be included (15.2.3.7 step 3).

http://codereview.chromium.org/463040/diff/5/1004
File test/mjsunit/object-create.js (right):

http://codereview.chromium.org/463040/diff/5/1004#newcode43
test/mjsunit/object-create.js:43: var Foo = { foo: function() { ctr++; }};
Peculiar naming convention -- Foo is not a function.

http://codereview.chromium.org/463040/diff/5/1004#newcode184
test/mjsunit/object-create.js:184: print("ok");
Excuse me?

[Erik Corry, 2009-12-08 10:20:00]

New version uploaded.

http://codereview.chromium.org/463040/diff/5/1006
File src/messages.js (right):

http://codereview.chromium.org/463040/diff/5/1006#newcode162
src/messages.js:162: value_and_getter_setter:      "Invalid property.  'value'
present on property with getter or setter",
On 2009/12/08 04:30:52, Christian Plesner Hansen wrote:
> This sounds confusing.  How about something along the lines of "A property
> cannot have both a value and accessors".

I'm matching the error messages from JSC here.  I agree they are not ideal.

http://codereview.chromium.org/463040/diff/5/1006#newcode163
src/messages.js:163: proto_object_or_null:         'Object prototype may only be
an Object or null',
On 2009/12/08 04:30:52, Christian Plesner Hansen wrote:
> Inconsistent string delimiters.

Fixed.

http://codereview.chromium.org/463040/diff/5/1005
File src/v8natives.js (right):

http://codereview.chromium.org/463040/diff/5/1005#newcode286
src/v8natives.js:286: if (desc === void 0) return false;
On 2009/12/08 04:30:52, Christian Plesner Hansen wrote:
> If you're deliberately avoiding IS_UNDEFINED please document why.  Otherwise
you
> should use IS_UNDEFINED.

Fixed.

http://codereview.chromium.org/463040/diff/5/1005#newcode316
src/v8natives.js:316: var enumerable = obj.enumerable;
On 2009/12/08 04:30:52, Christian Plesner Hansen wrote:
> This doesn't seem to match what's in the spec.  Shouldn't it be something like
> 
> var enum = false;
> if ('enumerable' in obj) {
>   enum = ToBoolean(obj.enumerable);
> }
> 
> Ditto, with variations, for every block below.  In any case |enumerable in
obj|
> certainly seems incorrect, it should be |"enumerable" in obj|.

Certainly it was wrong, thanks for pointing out the missing ToBoolean and quotes
in the 'in' expression.  The reason why I explicitly check for undefined is that
we don't have an inline cache for 'in' so in the case where the property is
present and not undefined we skip the 'in'.

http://codereview.chromium.org/463040/diff/5/1005#newcode360
src/v8natives.js:360: this.value_ = void 0;
On 2009/12/08 04:30:52, Christian Plesner Hansen wrote:
> The spec says nothing about these properties being present on property
> descriptors.  This is both a problem in and of itself but it also allows
people
> to put a property descriptor into an inconsistent state.  

These property descriptors are never exposed to the user.  The spec does not
give any details on how they should be implemented.  The FromPropertyDescriptor
method in 8.10.4 makes a new object with a well-defined structure which is
passed to the user.

Also, I can't find the
> style guide right now but I believe variables should be lowerCamelCase.  This
> applies throughout.

In general, use functionNamesLikeThis, variableNamesLikeThis,
ClassNamesLikeThis, EnumNamesLikeThis, methodNamesLikeThis, and
SYMBOLIC_CONSTANTS_LIKE_THIS. (For legacy code, method names should start with
uppercase).

Getters and setters for properties are not required. However, if they are used,
then getters must be named getFoo() and setters must be named setFoo(value).
(For Boolean getters, isFoo() is also acceptable, and often sounds more
natural.)

Private properties, variables, and methods (in files or classes) should be named
with a trailing underscore

I'll fix this.

http://codereview.chromium.org/463040/diff/5/1005#newcode440
src/v8natives.js:440: for (var key in props) {
On 2009/12/08 04:30:52, Christian Plesner Hansen wrote:
> This will include properties in the prototype chain of 'properties', only own
> properties should be included (15.2.3.7 step 3).

Good catch.

http://codereview.chromium.org/463040/diff/5/1004
File test/mjsunit/object-create.js (right):

http://codereview.chromium.org/463040/diff/5/1004#newcode43
test/mjsunit/object-create.js:43: var Foo = { foo: function() { ctr++; }};
On 2009/12/08 04:30:52, Christian Plesner Hansen wrote:
> Peculiar naming convention -- Foo is not a function.

Actually foo is very like a superclass and is used as one so I think it's OK
that it's named like a class.  The others got their naming convention by just
riding on Foo's coat-tails and they are now fixed.

http://codereview.chromium.org/463040/diff/5/1004#newcode184
test/mjsunit/object-create.js:184: print("ok");
On 2009/12/08 04:30:52, Christian Plesner Hansen wrote:
> Excuse me?

The JSC shell silently terminates on exceptions (at least the way I build it...)
so this is mighty convienient.  It doesn't seem to hurt.  I suppose I could
surround the whole thing with a try-catch, but then the non-optimizing compiler
wouldn't get tested.

[Lasse Reichstein, 2009-12-08 11:35:46]

Drive by comments.

http://codereview.chromium.org/463040/diff/5/1004
File test/mjsunit/object-create.js (right):

http://codereview.chromium.org/463040/diff/5/1004#newcode43
test/mjsunit/object-create.js:43: var Foo = { foo: function() { ctr++; }};
It's still not traditional JS notation to uppercase prototypes. How about
calling it "protoFoo"?

http://codereview.chromium.org/463040/diff/5/1004#newcode184
test/mjsunit/object-create.js:184: print("ok");
Please don't do (permanent) workarounds for the non-optimizing compiler. It
will, eventually, support the full syntax, so unless you expect to go back and
change this when it happens, just write it correctly now.
(If anything, you can wrap the test in a function and call the function in a
catch, and then run with --always-fast-compiler, so the function is compiled
with the fast compiler.)

[Erik Corry, 2009-12-08 13:52:23]

http://codereview.chromium.org/463040/diff/5/1004
File test/mjsunit/object-create.js (right):

http://codereview.chromium.org/463040/diff/5/1004#newcode184
test/mjsunit/object-create.js:184: print("ok");
On 2009/12/08 11:35:46, Lasse Reichstein wrote:
> Please don't do (permanent) workarounds for the non-optimizing compiler. It
> will, eventually, support the full syntax, so unless you expect to go back and
> change this when it happens, just write it correctly now.
> (If anything, you can wrap the test in a function and call the function in a
> catch, and then run with --always-fast-compiler, so the function is compiled
> with the fast compiler.)

The print statement is now gone.

[Christian Plesner Hansen, 2009-12-08 20:44:40]

Lgtm, with more comments.

http://codereview.chromium.org/463040/diff/5/1006
File src/messages.js (right):

http://codereview.chromium.org/463040/diff/5/1006#newcode162
src/messages.js:162: value_and_getter_setter:      "Invalid property.  'value'
present on property with getter or setter",
I am very strongly against giving lower quality error messages to match JSC. 
It's up to you to decide what to do but you should at least consider giving a
better message and rebaselining the test.

http://codereview.chromium.org/463040/diff/5/1005
File src/v8natives.js (right):

http://codereview.chromium.org/463040/diff/5/1005#newcode312
src/v8natives.js:312: throw MakeTypeError(property_desc_object, [obj]);
*Ahem*

http://codereview.chromium.org/463040/diff/5/1005#newcode316
src/v8natives.js:316: var enumerable = obj.enumerable;
How is this not premature optimization?

http://codereview.chromium.org/463040/diff/5/1005#newcode360
src/v8natives.js:360: this.value_ = void 0;
> These property descriptors are never exposed to the user.  The spec does not
> give any details on how they should be implemented.  The
FromPropertyDescriptor
> method in 8.10.4 makes a new object with a well-defined structure which is
> passed to the user.

Okay, ignore my comment then.

http://codereview.chromium.org/463040/diff/9/11#newcode327
src/v8natives.js:327: if (value !== void 0 || value in obj)
desc.set_value(value);
IS_UNDEFINED

http://codereview.chromium.org/463040/diff/9/11#newcode344
src/v8natives.js:344: if (set !== void 0 && typeof(set) !== 'function') {
IS_UNDEFINED

http://codereview.chromium.org/463040/diff/9/11#newcode367
src/v8natives.js:367: this.get_set_ = false;
Calling a boolean xxxSet is not a particularly obvious choice and in this case,
where xxx is 'get' and 'set', it is really confusing.  Consider hasGetter,
hasSetter instead.

http://codereview.chromium.org/463040/diff/9/10
File test/mjsunit/object-create.js (right):

http://codereview.chromium.org/463040/diff/9/10#newcode43
test/mjsunit/object-create.js:43: var foo_proto = { foo: function() { ctr++; }};
lowerCamelCase.