Refactor the check for generating inline constructors...

Refactor the check for generating inline constructors

Moved all the logic to a function on SharedFunctionInfo (including the flag check) to make things more readable.

Changed the check for setter to do a lookup for a named setter for each of the properties assigned in the constructor.

Added tests using accessors and interseptors set through the API.

Added fast case objects to the mjsunit test.

TEST=test/mjsunit/setter-on-constructor-prototype.js
TEST=test/cctest/test-api/SetterOnConstructorPrototype
TEST=test/cctest/test-api/InterceptorOnConstructorPrototype

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

[Mads Ager (chromium), 2010-02-17 13:36:32]

LGTM, I wonder if we should make a new function traversing the prototype once
looking for all of the names?

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

http://codereview.chromium.org/619006/diff/1/5#newcode4844
src/objects.cc:4844: name, &result, true);
Will this formatting overflow?

JSObject::cast(prototype)->LookupCallbackSetterInPrototypes(name,
                                                            &result,
                                                            true);

What is the limit on the number of simple assignments for which we generate
constructors.  This ends up traversing the whole prototype chain for each name. 
But maybe that isn't a problem.

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

http://codereview.chromium.org/619006/diff/1/7#newcode1374
src/objects.h:1374: void LookupCallbackSetterInPrototypes(String* name,
I guess this should just be called LookupCallbackSetter now.  Can we make the
bool argument an enum with values INCLUDE_THIS and DONT_INCLUDE_THIS and remove
the default arg?

http://codereview.chromium.org/619006/diff/1/2
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/619006/diff/1/2#newcode9634
test/cctest/test-api.cc:9634: ""
Remove empty lines?

http://codereview.chromium.org/619006/diff/1/2#newcode9686
test/cctest/test-api.cc:9686: ""
Remove?

[Mads Ager (chromium), 2010-02-17 13:36:33]

LGTM, I wonder if we should make a new function traversing the prototype once
looking for all of the names?

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

http://codereview.chromium.org/619006/diff/1/5#newcode4844
src/objects.cc:4844: name, &result, true);
Will this formatting overflow?

JSObject::cast(prototype)->LookupCallbackSetterInPrototypes(name,
                                                            &result,
                                                            true);

What is the limit on the number of simple assignments for which we generate
constructors.  This ends up traversing the whole prototype chain for each name. 
But maybe that isn't a problem.

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

http://codereview.chromium.org/619006/diff/1/7#newcode1374
src/objects.h:1374: void LookupCallbackSetterInPrototypes(String* name,
I guess this should just be called LookupCallbackSetter now.  Can we make the
bool argument an enum with values INCLUDE_THIS and DONT_INCLUDE_THIS and remove
the default arg?

http://codereview.chromium.org/619006/diff/1/2
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/619006/diff/1/2#newcode9634
test/cctest/test-api.cc:9634: ""
Remove empty lines?

http://codereview.chromium.org/619006/diff/1/2#newcode9686
test/cctest/test-api.cc:9686: ""
Remove?

[Søren Thygesen Gjesse, 2010-02-18 09:06:10]

Dropped the use of LookupCallbackSetterInPrototypes and did an explicit
traversal of the chain in CanGenerateInlineConstructor as I needed a check for
objects on the prototype chain not being in dictionary mode for the upcoming map
check in the inline constructor code anyway.

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

http://codereview.chromium.org/619006/diff/1/5#newcode4844
src/objects.cc:4844: name, &result, true);
On 2010/02/17 13:36:33, Mads Ager wrote:
> Will this formatting overflow?
> 
> JSObject::cast(prototype)->LookupCallbackSetterInPrototypes(name,
>                                                             &result,
>                                                             true);
> 
> What is the limit on the number of simple assignments for which we generate
> constructors.  This ends up traversing the whole prototype chain for each
name. 
> But maybe that isn't a problem.

Not calling LookupCallbackSetterInPrototypes here any more.

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

http://codereview.chromium.org/619006/diff/1/7#newcode1374
src/objects.h:1374: void LookupCallbackSetterInPrototypes(String* name,
On 2010/02/17 13:36:33, Mads Ager wrote:
> I guess this should just be called LookupCallbackSetter now.  Can we make the
> bool argument an enum with values INCLUDE_THIS and DONT_INCLUDE_THIS and
remove
> the default arg?

Dropped this change.

[Mads Ager (chromium), 2010-02-18 09:19:35]

LGTM!

http://codereview.chromium.org/619006/diff/6001/6005
File src/objects.cc (right):

http://codereview.chromium.org/619006/diff/6001/6005#newcode4838
src/objects.cc:4838: // the same names as are set by the inline constructor..
.. -> .

http://codereview.chromium.org/619006/diff/6001/6002
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/619006/diff/6001/6002#newcode9634
test/cctest/test-api.cc:9634: ""
Remove trailing empty strings?

http://codereview.chromium.org/619006/diff/6001/6002#newcode9686
test/cctest/test-api.cc:9686: ""
Remove trailing empty strings?

http://codereview.chromium.org/619006/diff/6001/6003
File test/mjsunit/setter-on-constructor-prototype.js (right):

http://codereview.chromium.org/619006/diff/6001/6003#newcode111
test/mjsunit/setter-on-constructor-prototype.js:111: //RunTest(true);
Code in comment - doesn't this work?

[Søren Thygesen Gjesse, 2010-02-18 09:42:51]

Sorry for the mess, fixed in r3894 (http://codereview.chromium.org/647007)

http://codereview.chromium.org/619006/diff/6001/6005
File src/objects.cc (right):

http://codereview.chromium.org/619006/diff/6001/6005#newcode4838
src/objects.cc:4838: // the same names as are set by the inline constructor..
On 2010/02/18 09:19:35, Mads Ager wrote:
> .. -> .

http://codereview.chromium.org/647007

http://codereview.chromium.org/619006/diff/6001/6002
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/619006/diff/6001/6002#newcode9634
test/cctest/test-api.cc:9634: ""
On 2010/02/18 09:19:35, Mads Ager wrote:
> Remove trailing empty strings?

http://codereview.chromium.org/647007

http://codereview.chromium.org/619006/diff/6001/6002#newcode9686
test/cctest/test-api.cc:9686: ""
On 2010/02/18 09:19:35, Mads Ager wrote:
> Remove trailing empty strings?

http://codereview.chromium.org/647007

http://codereview.chromium.org/619006/diff/6001/6003
File test/mjsunit/setter-on-constructor-prototype.js (right):

http://codereview.chromium.org/619006/diff/6001/6003#newcode111
test/mjsunit/setter-on-constructor-prototype.js:111: //RunTest(true);
On 2010/02/18 09:19:35, Mads Ager wrote:
> Code in comment - doesn't this work?

http://codereview.chromium.org/647007 (it did work).