Type check when upcasting PassOwnPtr/PassOwnArrayPtr to avoid possible function call ambiguousness

Type check when upcasting PassOwnPtr/PassOwnArrayPtr to avoid possible function call ambiguousness

This change is basically extends RefPtr EnsurePtrConvertibleArg pattern to OwnPtr and OwnArrayPtr smart pointers.

Please look at the following example:

class BaseClass {};
class DerivedClass: public BaseClass {};

void foo(PassOwnPtr<BaseClass>) {}
void foo(PassOwnPtr<int /* or whatever type*/>) {}

void fooClientFunction()
{
    OwnPtr<DerivedClass> ptr = adoptPtr(new DerivedClass);
    foo(ptr.release());
}

before this change such snippet would have lead to "error: call of overloaded 'foo(WTF::PassOwnPtr<DerivedClass>)' is ambiguous". 
This CL solves the problem.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157449

[Mikhail, 2013-08-29 11:55:56]

Could you please have a look?

[Nico, 2013-08-29 19:58:47]

This looks like a fine CL. Maybe you can include an example of bad code that
compiled before this change and doesn't with this change in the CL description?

https://codereview.chromium.org/23531003/diff/1/Source/wtf/PassRefPtr.h
File Source/wtf/PassRefPtr.h (right):

https://codereview.chromium.org/23531003/diff/1/Source/wtf/PassRefPtr.h#newco...
Source/wtf/PassRefPtr.h:28: #define EnsureRefPtrConvertibleArgDefn \
This is identical to EnsureOwnArrayPtrConvertibleArgDefn, right? Maybe some
define like this should be in TypeTraits instead?

https://codereview.chromium.org/23531003/diff/1/Source/wtf/TypeTraits.h
File Source/wtf/TypeTraits.h (left):

https://codereview.chromium.org/23531003/diff/1/Source/wtf/TypeTraits.h#oldco...
Source/wtf/TypeTraits.h:249: typename EnableIf<IsPointerConvertible<From,
To>::Value, ReturnType>::Type
…here it is, right? I don't see the value in removing the two defines here if
they are added back in in two other places.

[Mikhail, 2013-08-30 10:06:19]

On 2013/08/29 19:58:47, Nico wrote:
> This looks like a fine CL. Maybe you can include an example of bad code that
> compiled before this change and doesn't with this change in the CL
description?
> 
Please look at the following example:

class BaseClass {};
class DerivedClass: public BaseClass {};

void foo(PassOwnPtr<BaseClass>) {}
void foo(PassOwnPtr<int /* or whatever type*/>) {}

void fooClientFunction()
{
    OwnPtr<DerivedClass> ptr = adoptPtr(new DerivedClass);
    foo(ptr.release());
}

at the moment such snippet will lead to "error: call of overloaded
'foo(WTF::PassOwnPtr<DerivedClass>)' is ambiguous".
This CL will solve the problem.

[Nico, 2013-08-31 20:51:34]

I see, thanks for explaining. Can you add that example to the CL description,
please?

https://codereview.chromium.org/23531003/diff/1/Source/wtf/TypeTraits.h
File Source/wtf/TypeTraits.h (left):

https://codereview.chromium.org/23531003/diff/1/Source/wtf/TypeTraits.h#oldco...
Source/wtf/TypeTraits.h:249: typename EnableIf<IsPointerConvertible<From,
To>::Value, ReturnType>::Type
On 2013/08/29 19:58:47, Nico wrote:
> …here it is, right? I don't see the value in removing the two defines here if
> they are added back in in two other places.

Any thoughts on this comment?

[Mikhail, 2013-09-02 10:59:28]

On 2013/08/31 20:51:34, Nico wrote:
> I see, thanks for explaining. Can you add that example to the CL description,
> please?
> 
> https://codereview.chromium.org/23531003/diff/1/Source/wtf/TypeTraits.h
> File Source/wtf/TypeTraits.h (left):
> 
>
https://codereview.chromium.org/23531003/diff/1/Source/wtf/TypeTraits.h#oldco...
> Source/wtf/TypeTraits.h:249: typename EnableIf<IsPointerConvertible<From,
> To>::Value, ReturnType>::Type
> On 2013/08/29 19:58:47, Nico wrote:
> > …here it is, right? I don't see the value in removing the two defines here
if
> > they are added back in in two other places.
> 
> Any thoughts on this comment?

I totally agree with your comment and I think Patch Set 2 already addresses it.
Thanks for having a look!

[kouhei (in TOK), 2013-09-02 12:17:23]

https://codereview.chromium.org/23531003/diff/8001/Source/wtf/OwnPtrCommon.h
File Source/wtf/OwnPtrCommon.h (right):

https://codereview.chromium.org/23531003/diff/8001/Source/wtf/OwnPtrCommon.h#...
Source/wtf/OwnPtrCommon.h:34: EnsurePtrConvertibleArgDefn(typename
RemovePointer<From>::Type, typename RemovePointer<To>::Type)
I think we should use the macro in TypeTraits.h.

[Mikhail, 2013-09-02 12:27:26]

On 2013/09/02 12:17:23, kouhei wrote:
> https://codereview.chromium.org/23531003/diff/8001/Source/wtf/OwnPtrCommon.h
> File Source/wtf/OwnPtrCommon.h (right):
> 
>
https://codereview.chromium.org/23531003/diff/8001/Source/wtf/OwnPtrCommon.h#...
> Source/wtf/OwnPtrCommon.h:34: EnsurePtrConvertibleArgDefn(typename
> RemovePointer<From>::Type, typename RemovePointer<To>::Type)
> I think we should use the macro in TypeTraits.h.

To my mind EnsurePtrConvertibleArgDefn(typename RemovePointer<From>::Type,
typename RemovePointer<To>::Type) would look too long inside client code, do you
think it is worth putting a version of EnsurePtrConvertibleArgDefn that would
include 'RemovePointer<>::Type' to TypeTraits.h?

[Nico, 2013-09-02 17:09:34]

On 2013/09/02 12:27:26, mikhail.pozdnyakov wrote:
> On 2013/09/02 12:17:23, kouhei wrote:
> > https://codereview.chromium.org/23531003/diff/8001/Source/wtf/OwnPtrCommon.h
> > File Source/wtf/OwnPtrCommon.h (right):
> > 
> >
>
https://codereview.chromium.org/23531003/diff/8001/Source/wtf/OwnPtrCommon.h#...
> > Source/wtf/OwnPtrCommon.h:34: EnsurePtrConvertibleArgDefn(typename
> > RemovePointer<From>::Type, typename RemovePointer<To>::Type)
> > I think we should use the macro in TypeTraits.h.
> 
> To my mind EnsurePtrConvertibleArgDefn(typename RemovePointer<From>::Type,
> typename RemovePointer<To>::Type) would look too long inside client code, do
you
> think it is worth putting a version of EnsurePtrConvertibleArgDefn that would
> include 'RemovePointer<>::Type' to TypeTraits.h?

As far as I can tell you removed this define and then added the same identical
define back in two different places. I think that's worse than before. (But
maybe I misread the patch?)

[Mikhail, 2013-09-02 18:35:12]

https://codereview.chromium.org/23531003/diff/18001/Source/wtf/OwnPtr.h
File Source/wtf/OwnPtr.h (right):

https://codereview.chromium.org/23531003/diff/18001/Source/wtf/OwnPtr.h#newco...
Source/wtf/OwnPtr.h:63: ~OwnPtr()
Weird I don't have this change locally.

[Mikhail, 2013-09-02 18:41:52]

On 2013/09/02 17:09:34, Nico wrote:
> On 2013/09/02 12:27:26, mikhail.pozdnyakov wrote:
> > On 2013/09/02 12:17:23, kouhei wrote:
> > >
https://codereview.chromium.org/23531003/diff/8001/Source/wtf/OwnPtrCommon.h
> > > File Source/wtf/OwnPtrCommon.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23531003/diff/8001/Source/wtf/OwnPtrCommon.h#...
> > > Source/wtf/OwnPtrCommon.h:34: EnsurePtrConvertibleArgDefn(typename
> > > RemovePointer<From>::Type, typename RemovePointer<To>::Type)
> > > I think we should use the macro in TypeTraits.h.
> > 
> > To my mind EnsurePtrConvertibleArgDefn(typename RemovePointer<From>::Type,
> > typename RemovePointer<To>::Type) would look too long inside client code, do
> you
> > think it is worth putting a version of EnsurePtrConvertibleArgDefn that
would
> > include 'RemovePointer<>::Type' to TypeTraits.h?
> 
> As far as I can tell you removed this define and then added the same identical
> define back in two different places. I think that's worse than before. (But
> maybe I misread the patch?)

Now I'm not introducing any new macro, I was just a bit afraid that
EnsurePtrConvertibleArg in OwnPtr case wouldn't be easy to read. Could you
please take a look?

[Mikhail, 2013-09-05 15:35:34]

On 2013/09/02 18:41:52, mikhail.pozdnyakov wrote:
> On 2013/09/02 17:09:34, Nico wrote:
> > On 2013/09/02 12:27:26, mikhail.pozdnyakov wrote:
> > > On 2013/09/02 12:17:23, kouhei wrote:
> > > >
> https://codereview.chromium.org/23531003/diff/8001/Source/wtf/OwnPtrCommon.h
> > > > File Source/wtf/OwnPtrCommon.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/23531003/diff/8001/Source/wtf/OwnPtrCommon.h#...
> > > > Source/wtf/OwnPtrCommon.h:34: EnsurePtrConvertibleArgDefn(typename
> > > > RemovePointer<From>::Type, typename RemovePointer<To>::Type)
> > > > I think we should use the macro in TypeTraits.h.
> > > 
> > > To my mind EnsurePtrConvertibleArgDefn(typename RemovePointer<From>::Type,
> > > typename RemovePointer<To>::Type) would look too long inside client code,
do
> > you
> > > think it is worth putting a version of EnsurePtrConvertibleArgDefn that
> would
> > > include 'RemovePointer<>::Type' to TypeTraits.h?
> > 
> > As far as I can tell you removed this define and then added the same
identical
> > define back in two different places. I think that's worse than before. (But
> > maybe I misread the patch?)
> 
> Now I'm not introducing any new macro, I was just a bit afraid that
> EnsurePtrConvertibleArg in OwnPtr case wouldn't be easy to read. Could you
> please take a look?

OK, the treatment is here: https://codereview.chromium.org/23960005/

[Nico, 2013-09-06 22:11:50]

Sorry, was away for a while.

This now needs to be rebased since https://codereview.chromium.org/23960005/ has
landed, right? After you've done that, this patch lgtm.

Thank you for your patience!

[Mikhail, 2013-09-09 07:42:20]

On 2013/09/06 22:11:50, Nico wrote:
> Sorry, was away for a while.
No worries :)

> 
> This now needs to be rebased since https://codereview.chromium.org/23960005/
has
> landed, right? After you've done that, this patch lgtm.
Yeah, now it's gonna look much simpler.

> 
> Thank you for your patience!
Thank you!

[commit-bot: I haz the power, 2013-09-09 10:06:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/23531003/...

[commit-bot: I haz the power, 2013-09-09 11:14:33]

Change committed as 157449