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

Subject: Re: [Boost-users] [move] case study: simple cloning smart pointer
From: Jeffrey Lee Hellrung, Jr. (jeffrey.hellrung_at_[hidden])
Date: 2012-07-17 03:17:23

[Just a few suggestions...]

On Mon, Jul 16, 2012 at 4:54 AM, Krzysztof Czainski <1czajnik_at_[hidden]>wrote:

> Hello,
>
> In order to learn how to implement move semantics, I decided to write a
> simple cloning smart pointer (called ptr) with diagnostic information, and
> an example showing all constructor and assignment use cases I could think
> of. I wanted the pointer to be convertible (and movable) to pointers to
> base classes.
>
> I've prepared two versions of conversion constructors and assignment
> operators in ptr. In the first version I tried to write everything like
> Boost.Move docs for implementing Copyable and Movable classes suggest, and
> then added the conversion constructors and assignment operators. Then I
> wrote a second version, using the pass-by-value and swap idiom.
>
> Any comments about my attempts here are welcome. Did I get it right in
> both versions?
>
> The attached file swap_idiom.zip contains:
> ptr.hpp - the deffinition of the ptr class template
> main.cpp - example for ptr<>, showing all constructor and assignment use
> cases I could think of
> 03.txt, 03_by_value.txt, 0x.txt, 0x_by_value.txt - the output I got for
> both versions of ptr, in two versions of C++: 03 and 0x.
> For this test I used MinGW-4.5.0.
>
> Here's how the ptr<> class looks like (ptr.hpp) (note: {{{this}}} is how I
> mark code):
> {{{
> extern int ptr_s; // helper for diagnostics
> template < class T > T* clone_me( T const* p );
>
> template < class T >
> class ptr
> {
> public:
>
>     explicit ptr( T* p = 0 ) : p_(p) { ++ptr_s; std::cout << "_p "; }
>
>     // copy ctor
>     ptr( ptr const& b ) : p_( clone_me(b.get()) ) { ++ptr_s; std::cout <<
> "cp "; }
>
>     // move ctor
>     ptr( BOOST_RV_REF(ptr) b ) : p_( b.release() ) { ++ptr_s; std::cout <<
> "mp "; }
> }}}
>
> So far so good.
>

Agreed.

> 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.

    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 :)

> 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.
>
> The rest of ptr<> follows:
> {{{
>     ~ptr() { boost::checked_delete(p_); ++ptr_s; std::cout << "~p "; }
>
>     T* get() const { return p_; }
>
>     T* release() { T* r = p_; p_ = 0; return r; }
>
>     friend void swap( ptr& a, ptr& b )
>     {
>         boost::swap(a.p_,b.p_);
>         std::cout << "sp ";
>     }
>
>     // reset, operator* and ->
>
> private:
>
>     T* p_;
> };
> }}}
>
> Writing the conversion constructors and assignment operators in the second
> version was much simpler. While version 1 contains 6 functions, version 2
> contains only 2 functions. But is this always correct, and what is the cost
> of simplifying things? I tried to answer that question by writing the
> example (main.cpp). I started by preparing two diagnostic classes:
> {{{
> struct A
> {
>     static int a, c;
>     A() { ++a; ++c; cout << "_A "; }
>     A( A const& ) { ++a; ++c; cout << "cA "; }
>     virtual ~A() { ++a; --c; cout << "~A "; }
> };
> int A::a = 0;
> int A::c = 0;
> struct B : A
> {
>     static int b, d;
>     B() { ++b; ++d; cout << "_B "; }
>     B( B const& x ) : A(x) { ++b; ++d; cout << "cB "; }
>     ~B() { ++b; --d; cout << "~B "; }
> };
> int B::b = 0;
> int B::d = 0;
>
>  ptr<A> make_a()
> {
>     return ptr<A>( new A );
> }
>
> ptr<B> make_b()
> {
>     return ptr<B>( new B );
> }
> }}}
>
> Then some machinery for printing and zeroing the static variables
> incremented/decremented by corresponding constructors/destructors.
>
> {{{
> void trace( char const* msg )
> {
>     cout << endl << boost::format("%34s P:%d A:%d B:%d eA:%d eB:%d") % msg
> %
>             ptr_s % A::a % B::b % A::c % B::d << endl;
>     ptr_s = 0; A::a = 0; B::b = 0;
> }
>
> #define TRACE( x ) x; trace( #x );
> }}}
>
> And now main() itself, containing all use cases of ptr<> I could think of:
> {{{
> int main()
> {
>     {
>         TRACE( ptr<A> z )
>         TRACE( ptr<A> a( new A ) )
>         TRACE( ptr<B> b( new B ) )
>         TRACE( ptr<A> c( new B ) )
>         TRACE( ptr<A> d( a ) )
>         TRACE( ptr<A> e( b ) )
>         TRACE( ptr<A> f( boost::move(a) ) )
>         TRACE( ptr<A> g( boost::move(b) ) )
>         TRACE( ptr<B> h( new B ) )
>         TRACE( ptr<A> i( boost::move(c) ) )
>         TRACE( ptr<A> j( i ) ) // slice
>
>         TRACE( ptr<A> x( new A ) )
>         TRACE( x = f )
>         TRACE( x = h )
>         TRACE( x = boost::move(f) )
>         TRACE( x = boost::move(g) )
>         TRACE( x = boost::move(h) )
>         TRACE( x = i ) // slice
>     }
>     trace( "" );
>     {
>         TRACE( ptr<A> a = make_a() )
>         TRACE( ptr<A> b = make_b() )
>         TRACE( ptr<A> c( make_b() ) )
>         TRACE( a = make_a() )
>         TRACE( b = make_b() )
>     }
>     trace( "" );
> }
> }}}
>
> 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.

> C++0x: 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;
> - no unnecessary deep copies are introduced in either version.
>

If you want maximum performance in either C++03 or C++11, you're probably
going to have to (a) target the overload set specifically to the
presence/absence of rvalue references; and (b) in C++03, use both by-value
overloads as well as explicit RV_REF overloads, or use a type-erasure trick
to capture rvalues of move-emulation-enabled types. Yes, it can be a royal
pain; macros to automatically generate some commonly recurring overload
sets can help alleviate these issues.

Version 1: C++03 vs. C++0x:
> - 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 C++03, while
> while the deep copy is avoided in C++0x.
>
> Version 2: C++03 vs. C++0x:
> - no difference.
>
> General conclusion: 'pass-by-value and swap' idiom is cool ;-)
> Pros:
> - better move emulation,
> - simple implementation.
> Cons:
> - additional temporary objects, that are then swapped to the right place.
>
> Thanks for staying with me ;-)
>

Hope that provides with additional insight,

- Jeff