Make new Harmony constructors subclassing-friendly

Make new Harmony constructors subclassing-friendly

R=mstarzinger@chromium.org
CC=arv@chromium.org

[Dmitry Lomov (no reviews), 2013-08-03 15:10:40]

PTAL.

[arv (Not doing code reviews), 2013-08-04 16:44:28]

https://codereview.chromium.org/21924007/diff/3001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/21924007/diff/3001/src/collection.js#newcode52
src/collection.js:52: if (!IS_SET(this)) {
Remember that all of these needs to be changed to actually test that it is an
initialized Set and not just a Set.

var uninitializedSet = %NewObject(Set);
assertThrows(function() {
  Set.prototype.add.call(uninitializedSet, 'key');
}, TypeError);

At this point, the only way to get  an uninitialized Set is to use %NewObject so
and user code will not be able to trigger this. But once we expose @@create this
will need to be fixed.

https://codereview.chromium.org/21924007/diff/3001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/21924007/diff/3001/src/runtime.cc#newcode914
src/runtime.cc:914: ArrayIdToTypeAndSize(arrayId, &array_type, &element_size,
&name);
Can you move this code into the error cases? The common case is that there is no
error and we can skip all this extra work just to get the name that we are not
going to use.

https://codereview.chromium.org/21924007/diff/3001/src/runtime.cc#newcode970
src/runtime.cc:970: ArrayIdToTypeAndSize(arrayId, &array_type, &element_size,
&name);
Ditto

[Dmitry Lomov (no reviews), 2013-08-05 10:15:22]

https://codereview.chromium.org/21924007/diff/3001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/21924007/diff/3001/src/collection.js#newcode52
src/collection.js:52: if (!IS_SET(this)) {
On 2013/08/04 16:44:28, arv wrote:
> Remember that all of these needs to be changed to actually test that it is an
> initialized Set and not just a Set.
> 
> var uninitializedSet = %NewObject(Set);
> assertThrows(function() {
>   Set.prototype.add.call(uninitializedSet, 'key');
> }, TypeError);
> 
> At this point, the only way to get  an uninitialized Set is to use %NewObject
so
> and user code will not be able to trigger this. But once we expose @@create
this
> will need to be fixed.

Do you think we should fix all of these in this CL or we can do other error
checking in later CLs? (this whole @@create thing is a can of worms, obviously)

https://codereview.chromium.org/21924007/diff/3001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/21924007/diff/3001/src/runtime.cc#newcode914
src/runtime.cc:914: ArrayIdToTypeAndSize(arrayId, &array_type, &element_size,
&name);
On 2013/08/04 16:44:28, arv wrote:
> Can you move this code into the error cases? The common case is that there is
no
> error and we can skip all this extra work just to get the name that we are not
> going to use.

Done. Do you think it's cleaner that way? (Perf impact is non-existent of
course)

https://codereview.chromium.org/21924007/diff/3001/src/runtime.cc#newcode970
src/runtime.cc:970: ArrayIdToTypeAndSize(arrayId, &array_type, &element_size,
&name);
On 2013/08/04 16:44:28, arv wrote:
> Ditto
Ditto :)

[arv (Not doing code reviews), 2013-08-05 13:34:33]

https://codereview.chromium.org/21924007/diff/3001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/21924007/diff/3001/src/collection.js#newcode52
src/collection.js:52: if (!IS_SET(this)) {
On 2013/08/05 10:15:22, Dmitry Lomov (chromium) wrote:
> On 2013/08/04 16:44:28, arv wrote:
> > Remember that all of these needs to be changed to actually test that it is
an
> > initialized Set and not just a Set.
> > 
> > var uninitializedSet = %NewObject(Set);
> > assertThrows(function() {
> >   Set.prototype.add.call(uninitializedSet, 'key');
> > }, TypeError);
> > 
> > At this point, the only way to get  an uninitialized Set is to use
%NewObject
> so
> > and user code will not be able to trigger this. But once we expose @@create
> this
> > will need to be fixed.
> 
> Do you think we should fix all of these in this CL or we can do other error
> checking in later CLs? (this whole @@create thing is a can of worms,
obviously)

I think it is fine to do this later. It is not observable without %NewObject

https://codereview.chromium.org/21924007/diff/3001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/21924007/diff/3001/src/runtime.cc#newcode914
src/runtime.cc:914: ArrayIdToTypeAndSize(arrayId, &array_type, &element_size,
&name);
On 2013/08/05 10:15:22, Dmitry Lomov (chromium) wrote:
> On 2013/08/04 16:44:28, arv wrote:
> > Can you move this code into the error cases? The common case is that there
is
> no
> > error and we can skip all this extra work just to get the name that we are
not
> > going to use.
> 
> Done. Do you think it's cleaner that way? (Perf impact is non-existent of
> course)
> 

Sorry, I think my comment was bad advice. I didn't realize that this work needs
to be done in the common case anyway. Adding the name will be lost in the noise.
I didn't realize how much work is done on initialization anyway.

I'll let you determine which way you prefer. The old way is definitely cleaner.

[Dmitry Lomov (no reviews), 2013-08-05 14:07:51]

Ok upon some deliberation, I decided that it is too early to land this patch.

1. This patch does not have any support for methods other than constructors
being invoked on uninitialized objects.
2. We do not expose @@construct on JS objects anyway yet, so this patch solves a
problem that does not exists
3  Once we do expose @@construct, we need to think carefully what the design
would be (a strawman per Andreas's suggestion all un-initialized objects sstart
with the same instance type [say JS_BLUEPRINT] that transitions to correct
instance type after a call to constructor)

Thanks to Erik's change in https://code.google.com/p/v8/source/detail?r=16006,
we in a nice future-proof state now, which will let us move forward once we
start implementing subclassing support in earnest.

On 2013/08/05 13:34:33, arv wrote:
> https://codereview.chromium.org/21924007/diff/3001/src/collection.js
> File src/collection.js (right):
> 
> https://codereview.chromium.org/21924007/diff/3001/src/collection.js#newcode52
> src/collection.js:52: if (!IS_SET(this)) {
> On 2013/08/05 10:15:22, Dmitry Lomov (chromium) wrote:
> > On 2013/08/04 16:44:28, arv wrote:
> > > Remember that all of these needs to be changed to actually test that it is
> an
> > > initialized Set and not just a Set.
> > > 
> > > var uninitializedSet = %NewObject(Set);
> > > assertThrows(function() {
> > >   Set.prototype.add.call(uninitializedSet, 'key');
> > > }, TypeError);
> > > 
> > > At this point, the only way to get  an uninitialized Set is to use
> %NewObject
> > so
> > > and user code will not be able to trigger this. But once we expose
@@create
> > this
> > > will need to be fixed.
> > 
> > Do you think we should fix all of these in this CL or we can do other error
> > checking in later CLs? (this whole @@create thing is a can of worms,
> obviously)
> 
> I think it is fine to do this later. It is not observable without %NewObject
> 
> https://codereview.chromium.org/21924007/diff/3001/src/runtime.cc
> File src/runtime.cc (right):
> 
> https://codereview.chromium.org/21924007/diff/3001/src/runtime.cc#newcode914
> src/runtime.cc:914: ArrayIdToTypeAndSize(arrayId, &array_type, &element_size,
> &name);
> On 2013/08/05 10:15:22, Dmitry Lomov (chromium) wrote:
> > On 2013/08/04 16:44:28, arv wrote:
> > > Can you move this code into the error cases? The common case is that there
> is
> > no
> > > error and we can skip all this extra work just to get the name that we are
> not
> > > going to use.
> > 
> > Done. Do you think it's cleaner that way? (Perf impact is non-existent of
> > course)
> > 
> 
> Sorry, I think my comment was bad advice. I didn't realize that this work
needs
> to be done in the common case anyway. Adding the name will be lost in the
noise.
> I didn't realize how much work is done on initialization anyway.
> 
> I'll let you determine which way you prefer. The old way is definitely
cleaner.