abstract eternal into class

abstract eternal into class

R=rossberg@chromium.org, svenpanne@chromium.org
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=16316

[dcarney, 2013-08-12 14:59:51]

okay, successfully abstracted int, pls review

[rossberg, 2013-08-12 15:13:13]

Thanks. :)  LGTM, with nits.

https://chromiumcodereview.appspot.com/22795004/diff/1/src/api.cc
File src/api.cc (right):

https://chromiumcodereview.appspot.com/22795004/diff/1/src/api.cc#newcode679
src/api.cc:679: void V8::Eternalize(i::Isolate* isolate, i::Object** handle,
int* index) {
Is there a reason for changing this? I'd keep the functional version if
possible.

https://chromiumcodereview.appspot.com/22795004/diff/1/src/global-handles.h
File src/global-handles.h (right):

https://chromiumcodereview.appspot.com/22795004/diff/1/src/global-handles.h#n...
src/global-handles.h:400: void Create(Isolate* isolate, Object* object, int*
index);
Same here.

https://chromiumcodereview.appspot.com/22795004/diff/1/test/cctest/test-globa...
File test/cctest/test-global-handles.cc (right):

https://chromiumcodereview.appspot.com/22795004/diff/1/test/cctest/test-globa...
test/cctest/test-global-handles.cc:521: // Create an eternal with via the
constructor
Nit: typo?

[Sven Panne, 2013-08-13 06:11:18]

https://chromiumcodereview.appspot.com/22795004/diff/1/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/22795004/diff/1/include/v8.h#newcode486
include/v8.h:486: template<class S>
Why do we have a separate template parameter here? As it is, this signature
tells me: "It is OK to put any kind of value into an Eternal<T>, not just Ts." I
don't think this is right...

[dcarney, 2013-08-13 06:46:23]

https://chromiumcodereview.appspot.com/22795004/diff/1/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/22795004/diff/1/include/v8.h#newcode486
include/v8.h:486: template<class S>
On 2013/08/13 06:11:18, Sven Panne wrote:
> Why do we have a separate template parameter here? As it is, this signature
> tells me: "It is OK to put any kind of value into an Eternal<T>, not just Ts."
I
> don't think this is right...

As discussed offline, TYPE_CHECK is in implementation

https://chromiumcodereview.appspot.com/22795004/diff/1/src/api.cc
File src/api.cc (right):

https://chromiumcodereview.appspot.com/22795004/diff/1/src/api.cc#newcode679
src/api.cc:679: void V8::Eternalize(i::Isolate* isolate, i::Object** handle,
int* index) {
On 2013/08/12 15:13:13, rossberg wrote:
> Is there a reason for changing this? I'd keep the functional version if
> possible.

The assertion that you're not reinitializing an eternal is in Create().

https://chromiumcodereview.appspot.com/22795004/diff/1/test/cctest/test-globa...
File test/cctest/test-global-handles.cc (right):

https://chromiumcodereview.appspot.com/22795004/diff/1/test/cctest/test-globa...
test/cctest/test-global-handles.cc:521: // Create an eternal with via the
constructor
On 2013/08/12 15:13:13, rossberg wrote:
> Nit: typo?

fixed

[Sven Panne, 2013-08-21 10:20:22]

LGTM with a nit.

https://chromiumcodereview.appspot.com/22795004/diff/9001/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/22795004/diff/9001/include/v8.h#newcod...
include/v8.h:5686: V8::Eternalize(reinterpret_cast<internal::Isolate*>(isolate),
I think it would be nicer if we could do all this ugly casting in the actual
implementation (i.e. V8::Eternalize in the case at hand), unless we have a very
strong reason to do it in the header (which is not the case here).

I know that we already do this at tons of places in v8.h, but it is conceptually
ugly as hell and destroys any kind of layering.

[dcarney, 2013-08-26 09:41:32]

Committed patchset #3 manually as r16316 (presubmit successful).