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: Ion Gaztañaga (igaztanaga_at_[hidden])
Date: 2009-08-24 03:18:21

Thomas Klimpel escribió:
> 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.

Yes, but that wouldn't be the right thing. When writing move 
constructors one must first move base classes and then 
members.Otherwise, the base it's copied instead of moved. Anyway, I 
think this should be reflected better casting mx to base_t and then moved.

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

You are right.

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

You are right. clear() was added later in the design so there are some 
flaws regarding these issues. Thanks for spotting this.

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

This can lead to suprissing behaviours in the presence of some reference 
counted values:

vector<shared_ptr<T>> v1 (...);

shared_ptr<T> t1(new T(...));

v1.push_back(move(t1));

vector<shared_ptr<T>> v2 (...);

shared_ptr<T> t2(new T(...));

v2.push_back(move(t2));

//....

//transfer t2 pointee to v2
v1 = move(v2);
//..but t1's pointee is still alive.

The programmer has not required any value swapping just a "transfer" and 
I think the programmer expects all previous v1 values should have been 
destroyed. This has been discussed before, but I agree that we should 
have a consensus on these issues. Maybe committee members have some 
guidelines for move semantics.

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

Still waiting... ;-) Thanks for our comments,

Best,

Ion