Add a new C++ pointer wrapping API to External to not dilute the...

Add a new C++ pointer wrapping API to External to not dilute the
External::Cast.


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

[Kasper Lund, 2009-03-23 18:21:03]

LGTM.

http://codereview.chromium.org/52021/diff/1/3
File include/v8.h (right):

http://codereview.chromium.org/52021/diff/1/3#newcode1147
Line 1147: * The Wrap function V8 will return the most optimal Value object
wrapping the
Extra newline before 'The Wrap...'

http://codereview.chromium.org/52021/diff/1/3#newcode1148
Line 1148: * c++ void*. The type of the value is not guaranteed to be an
External object
C++ (capital C)

http://codereview.chromium.org/52021/diff/1/3#newcode1156
Line 1156: static void* Unwrap(Value* obj);
Why doesn't this take a Handle<Value>?

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

http://codereview.chromium.org/52021/diff/1/4#newcode2459
Line 2459: static Local<External> External_New_impl(void* data) {
What's up with all the underscores? We don't usually use those in function names
that have uppercase chars? Do you want this to be 'inline' too?

http://codereview.chromium.org/52021/diff/1/4#newcode2463
Line 2463: static void* External_Value_impl(i::Handle<i::Object> obj) {
Ditto.

http://codereview.chromium.org/52021/diff/1/4#newcode2492
Line 2492: return reinterpret_cast<void*>(i::Smi::cast(*obj)->value() <<
Consider computing the value << kAlignedxxx before the return and stuff it in a
local variable to improve readability.

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

http://codereview.chromium.org/52021/diff/1/2#newcode1350
Line 1350: char* char_ptr =
reinterpret_cast<char*>(v8::External::Unwrap(*zero));
Why aren't we just passing the handles here? Why does the Unwrap interface take
a Value*? Seems a bit weird to me.

[iposva, 2009-03-23 19:00:52]

http://codereview.chromium.org/52021/diff/1/3
File include/v8.h (right):

http://codereview.chromium.org/52021/diff/1/3#newcode1147
Line 1147: * The Wrap function V8 will return the most optimal Value object
wrapping the
On 2009/03/23 18:21:03, Kasper Lund wrote:
> Extra newline before 'The Wrap...'

Done.

http://codereview.chromium.org/52021/diff/1/3#newcode1148
Line 1148: * c++ void*. The type of the value is not guaranteed to be an
External object
On 2009/03/23 18:21:03, Kasper Lund wrote:
> C++ (capital C)

"It was this way when I got here." I copied the style from above. Changed all
"c++" -> "C++" to be consistent.

http://codereview.chromium.org/52021/diff/1/3#newcode1156
Line 1156: static void* Unwrap(Value* obj);
On 2009/03/23 18:21:03, Kasper Lund wrote:
> Why doesn't this take a Handle<Value>?

Mainly because I mirrored the Cast API. Done.

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

http://codereview.chromium.org/52021/diff/1/4#newcode2459
Line 2459: static Local<External> External_New_impl(void* data) {
On 2009/03/23 18:21:03, Kasper Lund wrote:
> What's up with all the underscores? We don't usually use those in function
names
> that have uppercase chars? Do you want this to be 'inline' too?

Removed the underscores. I am leaving the inline decision up to the C++
compiler.

http://codereview.chromium.org/52021/diff/1/4#newcode2463
Line 2463: static void* External_Value_impl(i::Handle<i::Object> obj) {
On 2009/03/23 18:21:03, Kasper Lund wrote:
> Ditto.

Ditto.

http://codereview.chromium.org/52021/diff/1/4#newcode2492
Line 2492: return reinterpret_cast<void*>(i::Smi::cast(*obj)->value() <<
On 2009/03/23 18:21:03, Kasper Lund wrote:
> Consider computing the value << kAlignedxxx before the return and stuff it in
a
> local variable to improve readability. 

Done.

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

http://codereview.chromium.org/52021/diff/1/2#newcode1350
Line 1350: char* char_ptr =
reinterpret_cast<char*>(v8::External::Unwrap(*zero));
On 2009/03/23 18:21:03, Kasper Lund wrote:
> Why aren't we just passing the handles here? Why does the Unwrap interface
take
> a Value*? Seems a bit weird to me.

Done.