Added ability to remove prototype from function. In this case, [[Construct]] ...

Added ability to remove prototype from function. In this case, [[Construct]] from function will not be allowed.
Added runtime function %FunctionRemovePrototype for this.
Removed prototypes from all builtin functions.
Some sputnik tests marked as fixed.
Added test to check builtins behavior.


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

[antonm, 2010-04-21 13:51:12]

Drive by question

http://codereview.chromium.org/1722003/diff/1/4
File src/objects.cc (right):

http://codereview.chromium.org/1722003/diff/1/4#newcode4935
src/objects.cc:4935: Object* JSFunction::RemovePrototype() {
is it fine that we create many maps---one per removed prototype?

[dgozman, 2010-04-21 14:04:52]

http://codereview.chromium.org/1722003/diff/1/4
File src/objects.cc (right):

http://codereview.chromium.org/1722003/diff/1/4#newcode4935
src/objects.cc:4935: Object* JSFunction::RemovePrototype() {
On 2010/04/21 13:51:12, antonm wrote:
> is it fine that we create many maps---one per removed prototype?

We could not modify the map, if it is used by another function. If natives
syntax is not allowed, it will be done ~100 times only for builtin functions.

[Mads Ager (chromium), 2010-04-22 08:36:00]

Looks like a good start.  I think we need to do more to the new JSFunction map. 
In particular, we need to remove the prototype accessor completely.  Maybe you
can allocate the 'JSFunction with no prototype' map up front and just change to
that map in the RemovePrototype method?

http://codereview.chromium.org/1722003/diff/1/8
File src/accessors.cc (right):

http://codereview.chromium.org/1722003/diff/1/8#newcode445
src/accessors.cc:445: if (function->map()->is_function_prototype_removed()) {
Maybe add a function->should_have_prototype() method?

Also, still having the GetPrototype accessor on the function means that the
function will have the prototype property but the value of the property will be
undefined. Is that the intended behavior? I think the intended behavior is that
the prototype property is not there at all?

In other words: "'prototype' in eval" should evaluate to false, right?

I think we need a special kind of JSFunction that does not have this accessor.

http://codereview.chromium.org/1722003/diff/1/5
File src/bootstrapper.cc (right):

http://codereview.chromium.org/1722003/diff/1/5#newcode339
src/bootstrapper.cc:339: Factory::NewFunctionWithPrototype(symbol,
Can we make NewFunctionWithPrototype handle the case where we explicitly do not
want a function prototype? That way, maybe we can avoid having a
RemovePrototype() method and just give the function the correct map with the bit
set when it is created? Also, we probably need to make sure that the function
does not have the prototype accessor at all.

I guess you still need something like RemovePrototype so you can remove the
prototype from the funtions in the JS natives...

http://codereview.chromium.org/1722003/diff/1/7
File src/objects.h (right):

http://codereview.chromium.org/1722003/diff/1/7#newcode2860
src/objects.h:2860: inline void set_function_prototype_removed(bool value);
Maybe this should be called function_should_have_prototype?

[antonm, 2010-04-22 14:50:58]

Sorry, forgot to hit publish all my drafts yesterday.  But with Mads' response
it's now somewhat outdated.

http://codereview.chromium.org/1722003/diff/1/4
File src/objects.cc (right):

http://codereview.chromium.org/1722003/diff/1/4#newcode4935
src/objects.cc:4935: Object* JSFunction::RemovePrototype() {
On 2010/04/21 14:04:52, dgozman wrote:
> On 2010/04/21 13:51:12, antonm wrote:
> > is it fine that we create many maps---one per removed prototype?
> 
> We could not modify the map, if it is used by another function. If natives
> syntax is not allowed, it will be done ~100 times only for builtin functions.

I am not suggesting to modify map inplace, I just ask if we should cache new map
and use it for other functions with the same map as well.

[antonm, 2010-04-26 16:32:43]

Mostly minor comments, let's hear what Erik and Mads say.

http://codereview.chromium.org/1722003/diff/23001/24013
File src/ic.cc (right):

http://codereview.chromium.org/1722003/diff/23001/24013#newcode619
src/ic.cc:619: JSFunction::cast(*object)->should_have_prototype()) {
why this change?  can LoadIC_FunctionPrototype cope with prototype-less
functions?

http://codereview.chromium.org/1722003/diff/23001/24003
File src/objects.cc (right):

http://codereview.chromium.org/1722003/diff/23001/24003#newcode4936
src/objects.cc:4936: ASSERT(map() ==
context()->global_context()->function_map());
why we're sure map() will always identical to function_map()?  Is it possible to
invoke this method after we changed somehow function's map (say, assigned a
property to it)?

http://codereview.chromium.org/1722003/diff/23001/24005
File src/runtime.cc (right):

http://codereview.chromium.org/1722003/diff/23001/24005#newcode6558
src/runtime.cc:6558: // case generated code bailouts here, since function has
not initial_map.
has /no/ initial_map

[dgozman, 2010-04-26 16:47:46]

http://codereview.chromium.org/1722003/diff/23001/24013
File src/ic.cc (right):

http://codereview.chromium.org/1722003/diff/23001/24013#newcode619
src/ic.cc:619: JSFunction::cast(*object)->should_have_prototype()) {
On 2010/04/26 16:32:43, antonm wrote:
> why this change?  can LoadIC_FunctionPrototype cope with prototype-less
> functions?

LoadIC_FunctionPrototype will always miss, since the function doesn't have
initial_map.
Also, special accessor is used here (in return statement), which should not be
used.

http://codereview.chromium.org/1722003/diff/23001/24003
File src/objects.cc (right):

http://codereview.chromium.org/1722003/diff/23001/24003#newcode4936
src/objects.cc:4936: ASSERT(map() ==
context()->global_context()->function_map());
On 2010/04/26 16:32:43, antonm wrote:
> why we're sure map() will always identical to function_map()?  Is it possible
to
> invoke this method after we changed somehow function's map (say, assigned a
> property to it)?

This method is only invoked for JS builtins, which have initial function maps.
I'm not sure whether it's reasonable to work in the other case, since there is
no such usage.

http://codereview.chromium.org/1722003/diff/23001/24005
File src/runtime.cc (right):

http://codereview.chromium.org/1722003/diff/23001/24005#newcode6558
src/runtime.cc:6558: // case generated code bailouts here, since function has
not initial_map.
On 2010/04/26 16:32:43, antonm wrote:
> has /no/ initial_map

Done.

[antonm, 2010-04-26 17:09:14]

http://codereview.chromium.org/1722003/diff/23001/24013
File src/ic.cc (right):

http://codereview.chromium.org/1722003/diff/23001/24013#newcode619
src/ic.cc:619: JSFunction::cast(*object)->should_have_prototype()) {
On 2010/04/26 16:47:46, dgozman wrote:
> On 2010/04/26 16:32:43, antonm wrote:
> > why this change?  can LoadIC_FunctionPrototype cope with prototype-less
> > functions?
> 
> LoadIC_FunctionPrototype will always miss, since the function doesn't have
> initial_map.
> Also, special accessor is used here (in return statement), which should not be
> used.
> 
> 

okay

http://codereview.chromium.org/1722003/diff/23001/24003
File src/objects.cc (right):

http://codereview.chromium.org/1722003/diff/23001/24003#newcode4936
src/objects.cc:4936: ASSERT(map() ==
context()->global_context()->function_map());
On 2010/04/26 16:47:46, dgozman wrote:
> On 2010/04/26 16:32:43, antonm wrote:
> > why we're sure map() will always identical to function_map()?  Is it
possible
> to
> > invoke this method after we changed somehow function's map (say, assigned a
> > property to it)?
> 
> This method is only invoked for JS builtins, which have initial function maps.
> I'm not sure whether it's reasonable to work in the other case, since there is
> no such usage.

I understand that.  I wonder if we should play safer here.

[Erik Corry, 2010-04-27 13:02:12]

http://codereview.chromium.org/1722003/diff/23001/24004
File src/bootstrapper.cc (right):

http://codereview.chromium.org/1722003/diff/23001/24004#newcode252
src/bootstrapper.cc:252: bool add_prototype_property,
Boolean arguments are very hard to read at the call sites.  I suggest making
enums for this eg.
enum AddPrototypeProperty { DONT_ADD_PROTOTYPE_PROPERTY,
ADD_PROTOTYPE_PROPERTY};
I usually put the negative one first so they silently evaluate to false in case
I miss somewhere where it is used as a boolean.

http://codereview.chromium.org/1722003/diff/23001/24004#newcode254
src/bootstrapper.cc:254: bool make_prototype_enumerable = false);
The style guide deprecates default arguments unless it is very obvious what the
default should be.  I don't find this one obvious so I think we should get rid
of the default.  I realize this was like this before you started changing the
code.

http://codereview.chromium.org/1722003/diff/23001/24004#newcode418
src/bootstrapper.cc:418: // would not be used as constructors.
would not -> can not

http://codereview.chromium.org/1722003/diff/23001/24006
File src/objects.h (right):

http://codereview.chromium.org/1722003/diff/23001/24006#newcode2860
src/objects.h:2860: inline void set_function_without_prototype(bool value);
It is better to name booleans in a positive way so that we don't end up with
double negation:

set_function_without_prototype(false);  // Doesn't not have proto.

http://codereview.chromium.org/1722003/diff/23001/24001
File test/sputnik/sputnik.status (right):

http://codereview.chromium.org/1722003/diff/23001/24001#newcode56
test/sputnik/sputnik.status:56: 
Nice!

[dgozman, 2010-04-27 13:41:47]

http://codereview.chromium.org/1722003/diff/23001/24004
File src/bootstrapper.cc (right):

http://codereview.chromium.org/1722003/diff/23001/24004#newcode252
src/bootstrapper.cc:252: bool add_prototype_property,
On 2010/04/27 13:02:12, Erik Corry wrote:
> Boolean arguments are very hard to read at the call sites.  I suggest making
> enums for this eg.
> enum AddPrototypeProperty { DONT_ADD_PROTOTYPE_PROPERTY,
> ADD_PROTOTYPE_PROPERTY};
> I usually put the negative one first so they silently evaluate to false in
case
> I miss somewhere where it is used as a boolean.

Done.

http://codereview.chromium.org/1722003/diff/23001/24004#newcode254
src/bootstrapper.cc:254: bool make_prototype_enumerable = false);
On 2010/04/27 13:02:12, Erik Corry wrote:
> The style guide deprecates default arguments unless it is very obvious what
the
> default should be.  I don't find this one obvious so I think we should get rid
> of the default.  I realize this was like this before you started changing the
> code.

Done.

http://codereview.chromium.org/1722003/diff/23001/24004#newcode418
src/bootstrapper.cc:418: // would not be used as constructors.
On 2010/04/27 13:02:12, Erik Corry wrote:
> would not -> can not

Done.

http://codereview.chromium.org/1722003/diff/23001/24006
File src/objects.h (right):

http://codereview.chromium.org/1722003/diff/23001/24006#newcode2860
src/objects.h:2860: inline void set_function_without_prototype(bool value);
On 2010/04/27 13:02:12, Erik Corry wrote:
> It is better to name booleans in a positive way so that we don't end up with
> double negation:
> 
> set_function_without_prototype(false);  // Doesn't not have proto.

Done.

[Mads Ager (chromium), 2010-04-28 10:40:04]

LGTM

http://codereview.chromium.org/1722003/diff/38001/39004
File src/bootstrapper.cc (right):

http://codereview.chromium.org/1722003/diff/38001/39004#newcode478
src/bootstrapper.cc:478: // Allocate a distinct prototype for this function map,
so it will not add
Make the comment a bit more explicit? How about:

this function map -> the function map for functions without prototypes?

http://codereview.chromium.org/1722003/diff/38001/39009
File test/mjsunit/function-without-prototype.js (right):

http://codereview.chromium.org/1722003/diff/38001/39009#newcode1
test/mjsunit/function-without-prototype.js:1: // Copyright 2008 the V8 project
authors. All rights reserved.
2010

[dgozman, 2010-04-28 11:16:59]

http://codereview.chromium.org/1722003/diff/38001/39004
File src/bootstrapper.cc (right):

http://codereview.chromium.org/1722003/diff/38001/39004#newcode478
src/bootstrapper.cc:478: // Allocate a distinct prototype for this function map,
so it will not add
On 2010/04/28 10:40:04, Mads Ager wrote:
> Make the comment a bit more explicit? How about:
> 
> this function map -> the function map for functions without prototypes?

Done.

http://codereview.chromium.org/1722003/diff/38001/39009
File test/mjsunit/function-without-prototype.js (right):

http://codereview.chromium.org/1722003/diff/38001/39009#newcode1
test/mjsunit/function-without-prototype.js:1: // Copyright 2008 the V8 project
authors. All rights reserved.
On 2010/04/28 10:40:04, Mads Ager wrote:
> 2010

Done.