Re: [Boost-users] [move] case study: simple cloning smart pointer

Subject: Re: [Boost-users] [move] case study: simple cloning smart pointer
From: Krzysztof Czainski (1czajnik_at_[hidden])
Date: 2012-07-17 05:06:05

2012/7/17 Jeffrey Lee Hellrung, Jr. <jeffrey.hellrung_at_[hidden]>

> [Just a few suggestions...]
>
[snip]

> This is where the hard part comes in - the conversion constructors and
>> assignment operators. First version looks like this:
>>
>> {{{
>> private:
>>
>> #if !defined(BY_VALUE)
>>
>>     BOOST_COPYABLE_AND_MOVABLE(ptr)
>>
>> public:
>>
>>     // generalized copy ctor for pointers to derived
>>      template < class U >
>>     ptr( ptr<U> const& b,
>>             typename boost::enable_if< boost::is_convertible<U*,T*>
>> >::type* = 0 )
>>     : p_( clone_me(b.get()) ) { ++ptr_s; std::cout << "Cp "; }
>>
>>     // generalized move ctor for pointers to derived
>>     template < class U >
>>     ptr( BOOST_RV_REF(ptr<U>) b,
>>             typename boost::enable_if< boost::is_convertible<U*,T*>
>> >::type* = 0 )
>>     : p_( b.release() ) { ++ptr_s; std::cout << "Mp "; }
>>
>
> I think you'll generally do better with the conversion copy constructor
> taking by-value and relying on RVO (in C++03). (Implicit) Rvalues of ptr<U>
> won't be moved into the newly constructed ptr<T> as written above. Note
> that the RV_REF overload is still useful even if you change the conversion
> copy constructor to taking by-value. Of course, in C++11, you can't have
> both by-value and rvalue reference overloads :(
>
>     ptr& operator=( BOOST_COPY_ASSIGN_REF(ptr) b )
>>     {
>>         T* tmp = clone_me(b.get()); // this can throw
>>         boost::checked_delete(p_);
>>         p_ = tmp;
>>         return *this;
>>     }
>>
>>     ptr& operator=( BOOST_RV_REF(ptr) b )
>>     {
>>         boost::checked_delete(p_);
>>         p_ = b.release();
>>         return *this;
>>     }
>>
>>     template < class U >
>>     typename boost::enable_if< boost::is_convertible<U*,T*>, ptr& >::type
>>     operator=( ptr<U> const& b )
>>     {
>>         T* tmp = clone_me(b.get()); // this could throw
>>         boost::checked_delete(p_);
>>         p_ = tmp;
>>         return *this;
>>     }
>>
>
> Probably want to disable this overload when U == T. And, again, by-value
> is probably what you want in C++03.
>

Oh yeah, right, I will add that condition. Thanks.

    template < class U >
>>     typename boost::enable_if< boost::is_convertible<U*,T*>, ptr& >::type
>>     operator=( BOOST_RV_REF(ptr<U>) b )
>>     {
>>         boost::checked_delete(p_);
>>         p_ = b.release();
>>         return *this;
>>     }
>> }}}
>> This was long, and contains some tricky places, like the one commented
>> 'this could throw'. Did I even get it right? I think so, but I'm just
>> beginning to learn about move semantics here ;-)
>>
>
> I admit, I didn't really look at the bodies, so no comments regarding
> correctness there :)
>
[snip]

> And now for the costs. I compiled the attached code with MinGW-4.5.0 in 4
>> configurations: -std=c++0x disabled/enabled, and version 1/2. I compared
>> the program output to derive the conclusions:
>>
>> C++03: version 1 vs. version 2:
>> - whenever a conversion is needed (about half of the use cases above),
>> version 2 introduces an additional temporary ptr<> object (without a deep
>> copy); in one case it's 2 additional temporary ptr<>s;
>> - the last 4 use cases {{{ ptr<A> b = make_b(); ptr<A> c( make_b() ); a =
>> make_a(); b = make_b(); }}} introduce a deep copy in version 1, while while
>> the deep copy is avoided in version 2.
>>
>
> I'm pretty sure these extraneous deep copies would be avoided if you make
> the changes I suggested above.
>

You are right, and the second version (when BY_VALUE is defined) is my
attempt at solving that as you suggest. Please, could you have a look
again, and let me know if this is what you have in mind:

> Anyway, next comes the second version: implementing conversion
> constructors and assignment operators in terms of pass-by-value and swap.
> Note the use of BOOST_COPYABLE_AND_MOVABLE_ALT macro so that an
> operator=(ptr&) isn't inserted.
> {{{
> #else // BY_VALUE
>
>     BOOST_COPYABLE_AND_MOVABLE_ALT(ptr)
>
> public:
>
>     // generalized copy/move constructor implemented by pass-by-value &
> steal
>     template < class U >
>     ptr( ptr<U> b,
>             typename boost::enable_if< boost::is_convertible<U*,T*>
> >::type* = 0 )
>     : p_( b.release() ) { ++ptr_s; std::cout << "Vp "; }
>
>     // assignment - works for all types convertible to ptr
>     ptr& operator=( ptr b )
>     {
>         swap(*this,b);
>         return *this;
>     }
>
> #endif // BY_VALUE
> }}}
> Compared to the first version, this is really simple. I see no tricky
> parts here.
>
> [snip]

> Hope that provides with additional insight,
> - Jeff
>

Thank you for your reply, Jeff.
Kris