Add Persistent::Upcast. (issue 16206014)

[ma...@chromium.org]

Reviewers: dcarney, Sven Panne,

Message:
dcarney, svenpanne, ptal.

Description:
Add Persistent::Upcast.

It's needed for upcasting Persistent<Object> to Persistent<Value> after
handlepocalypse. (Value::Cast doesn't exist so we cannot use As or Cast.)

BUG=

Please review this at https://codereview.chromium.org/16206014/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M include/v8.h
M test/cctest/test-api.cc


Index: include/v8.h
diff --git a/include/v8.h b/include/v8.h
index
28a63f092efb5a7831ccd5a891a9cffddbf4f655..49dc25b2ad99429c77b4644397e3e78ff61fb62c
100644
--- a/include/v8.h
+++ b/include/v8.h
@@ -594,6 +594,12 @@ template <class T> class Persistent // NOLINT
}
#endif

+ template <class S> V8_INLINE(
+ static Persistent<T>& Upcast(Persistent<S>& that)) { // NOLINT
+ TYPE_CHECK(T, S);
+ return reinterpret_cast<Persistent<T>&>(that);
+ }
+
V8_DEPRECATED(static Persistent<T> New(Handle<T> that));

/**
Index: test/cctest/test-api.cc
diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index
cb3a38eee8e95f66dc294dce3dc5197050bc630e..d8760e8e8656c3669a4bfcf4523d82bc24aeab8f
100644
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -2883,6 +2883,19 @@ THREADED_TEST(ClearAndLeakGlobal) {
}


+THREADED_TEST(GlobalHandleUpcast) {
+ v8::Isolate* isolate = v8::Isolate::GetCurrent();
+ v8::HandleScope scope(isolate);
+ v8::Local<String> local = v8::Local<String>::New(v8_str("str"));
+ v8::Persistent<String> global_string(isolate, local);
+ v8::Persistent<Value>& global_value =
+ v8::Persistent<Value>::Upcast(global_string);
+ CHECK(global_value->IsString());
+ CHECK(global_string == v8::Persistent<String>::Cast(global_value));
+ global_string.Dispose();
+}
+
+
THREADED_TEST(LocalHandle) {
v8::HandleScope scope(v8::Isolate::GetCurrent());
v8::Local<String> local = v8::Local<String>::New(v8_str("str"));

[sven...@chromium.org]

https://codereview.chromium.org/16206014/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/16206014/diff/1/include/v8.h#newcode598
include/v8.h:598: static Persistent<T>& Upcast(Persistent<S>& that)) {
// NOLINT
How does this relate to Persistent::Cast? It looks like it's the same,
but with an additional TYPE_CHECK. Do we really need this plethora of
casting mehods (Cast/As/Upcast)? This is currently a totally
incomprehensible mess... :-/

Apart from that a tiny formatting nit: Breaking the line before
V8_INLINE makes the formatting less surprising.

https://codereview.chromium.org/16206014/

[ma...@chromium.org]

On 2013/06/05 07:08:49, Sven Panne wrote:
> https://codereview.chromium.org/16206014/diff/1/include/v8.h
> File include/v8.h (right):

> https://codereview.chromium.org/16206014/diff/1/include/v8.h#newcode598
> include/v8.h:598: static Persistent<T>& Upcast(Persistent<S>& that)) { //
NOLINT
> How does this relate to Persistent::Cast? It looks like it's the same, but
with
> an additional TYPE_CHECK. Do we really need this plethora of casting
> mehods
> (Cast/As/Upcast)? This is currently a totally incomprehensible mess... :-/

Like the description says: "Value::Cast doesn't exist so we cannot use As or
Cast."

Both As and Cast end up trying to call Value::Cast, whereas Upcast doesn't.
Instead it checks that the types are fine, and then just reinterpret_casts.

https://codereview.chromium.org/16206014/

[sven...@chromium.org]

Why don't we add Value::Cast? (I haven't thought deeply about this.) As it
is, I
don't want to obfuscate Persistent even further, 2 casting functions are not
good, 3 different casting functions are very bad. Please remember that we
are
talking about a public API here, so we should be careful what we do here,
people
might actually start using it...

https://codereview.chromium.org/16206014/