Re: [boost] [move][container] Review Request (new versions of Boost.Move and Boost.Container in sandbox and vault)

Subject: Re: [boost] [move][container] Review Request (new versions of Boost.Move and Boost.Container in sandbox and vault)
From: Thomas Klimpel (Thomas.Klimpel_at_[hidden])
Date: 2009-08-23 10:15:01

After looking at move/boost/move/move.hpp, I was interested in how movable types are actually supposed to behave, so I decided to give move/boost/container/vector.hpp a mini review (I just searched for "BOOST_" and "move", and reviewed the code I found that way):

   //! <b>Effects</b>: Move constructor. Moves mx's resources to *this.
   //!
   //! <b>Throws</b>: If allocator_type's copy constructor throws.
   //!
   //! <b>Complexity</b>: Constant.
   vector(BOOST_RV_REF(vector) mx)
      :  base_t(boost::move(mx))
   {  this->swap(mx);   }

I would say that if an automatic "bad smell" detection tool like Coverity would flag the above code as dangerous, it wouldn't be completely wrong. After mx was used via boost::move(mx), it is not supposed to be used again. So I think it would be better to write

   vector(BOOST_RV_REF(vector) mx)
      :  base_t(mx)
   {  this->swap(mx);   }

which is also less confusing for a human reader.

   //! <b>Effects</b>: Move assignment. All mx's values are transferred to *this.
   //!
   //! <b>Postcondition</b>: x.empty(). *this contains a the elements x had
   //!   before the function.
   //!
   //! <b>Throws</b>: If allocator_type's copy constructor throws.
   //!
   //! <b>Complexity</b>: Constant.
   vector& operator=(BOOST_RV_REF(vector) x)
   {
      if (&x != this){
         this->swap(x);
         x.clear();
      }
      return *this;
   }

I would argue that the implementation is not consistent with the documentation. The x.clear() statement can cause up to n destructors to be called, violating the "<b>Complexity</b>: Constant." claim.

The x.clear() statement also has a bad interaction with the "has_trivial_destructor_after_move" template specialization at the end of the vector.hpp file:

namespace boost {

//!has_trivial_destructor_after_move<> == true_type
//!specialization for optimizations
template <class T, class A>
struct has_trivial_destructor_after_move<boost::container::vector<T, A> >
{
   static const bool value = has_trivial_destructor<A>::value;
};

}

This code claims that boost::container::vector has a trivial destructor after move if the allocator has a trivial destructor after move. But this is not true, since x.clear() only called the destructors of the contained objects and set the vector size to 0, but m_start and m_capacity are still unchanged and the memory of the vector is still allocated.

So a fixed version could look like

   //! <b>Postcondition</b>: x.empty() && !x.capacity(). *this contains a the elements x had
   //!   before the function.
...
   //! <b>Complexity</b>: Linear to the number of elements in the vector (destructor calls).
   vector& operator=(BOOST_RV_REF(vector) x)
   {
      if (&x != this){
         this->swap(x);
         x.clear();
         shrink_to_fit();
      }
      return *this;
   }

But one can wonder whether it would not be better to completely omit the x.clear() statement. After all, x is ("equivalent to") an r-value, so it can be treated as if it will be destroyed immediately after the function call. It's the responsibility of the user that tags an function argument as r-value by using boost::move to ensure that it can be treated just like an r-value.

As you probably remember, I wondered whether I'm allowed to implement my move assignment operators as a simple swap. The answer of Frank Mori Hess convinced me that I'm allowed to do this.

Frank Mori Hess wrote:
> Thomas Klimpel wrote:
> > I hope I can omit the temporary, because two calls to swap are less
> > efficient than a single call to swap. I can't find any second thought that
> > forces me to create a temporary. Do I miss something?
>
> I don't think you need to.  The temporary is useful if you want to always give
> moved-from objects a specific state (presumably the move constructor does
> this).  But the standard is looser, it only requires the moved-from object
> doesn't violate any of the class invariants.

The argument against this was that it might be quite surprising to the user.

Ion Gaztañaga wrote:
> For some types (shared_ptr,
> containers...) a simple swap can lead to unexpected behavior because the
> programmer might not expect any value transference:
>
> shared_ptr<X> a(...), b(..);
> //Should b take ownership of a's resources?
> //That might be quite surprising.
> a = boost::move(b);

So my position is that the user better be prepared to expect that "a = boost::move(b); " can be implemented as "a.swap(b)", because move assignment is about efficiency, and a.swap(b) is often the most efficient implementation of move assignment.

I'm still looking forward for Boost.Move being scheduled for review.

Regards,
Thomas