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

Subject: Re: [boost] [move][container] Review Request (new versions ofBoost.Move and Boost.Container in sandbox and vault)
From: Howard Hinnant (howard.hinnant_at_[hidden])
Date: 2009-09-07 19:27:34

On Sep 7, 2009, at 6:50 PM, David Abrahams wrote:

>
> on Mon Sep 07 2009, Howard Hinnant <howard.hinnant-AT-gmail.com>  
> wrote:
>
>>> Do we think swap(x,y) is also an error when x == y?
>>
>> This is a decent argument to not assert in self-move-assignment.
>
> And not to call it undefined behavior.  self-std:swapping was  
> allowed in
> C++03 and I don't think we can break that.
>
>> But this is also almost certainly harmless:
>>
>> #include <algorithm>
>> #include <iostream>
>>
>> class A
>> {
>>    int data_;
>> public:
>>    explicit A(int data = 0) : data_(data) {}
>>    A(A&& a) : data_(a.data_) {a.data_ = 0;}
>>    A& operator=(A&& a) {data_ = a.data_; a.data_ = 0; return *this;}
>>    friend std::ostream& operator<<(std::ostream& os, const A& a)
>>    {
>>        return os << a.data_;
>>    }
>> };
>>
>> int main()
>> {
>>    A a(5);
>>    std::cout << "Before swap a = " << a << '\n';
>>    std::swap(a, a);
>>    std::cout << "After swap a = " << a << '\n';
>> }
>>
>> Before swap a = 5
>> After swap a = 5
>
> Yes, it's harmless, but I don't see how it's a "but." I must be  
> missing
> your point, because that example seems to support my argument.
>
>> But if I caught my own code doing a self-swap, yeah, I would treat it
>> as a bug and correct it.  For example, every time I've written  
>> reverse
>> (which does nothing but swap x and y while x and y move closer to  
>> each
>> other in the sequence), I'm careful to break out of the loop before  
>> &x
>> == &y.
>
> Yes, but if it's reverse's job to avoid that, then it's also swap's
> job (both are algorithms, neh?)  So should we put a self-swap test  
> there?

No, I don't think so.

std::swap will only do a self-move-assignment using a moved-from  
object.  Of course vector will never use this definition.  But if it  
did, then:

template <class _Tp, class _Allocator>
inline
vector<_Tp, _Allocator>&
vector<_Tp, _Allocator>::operator=(vector&& __x)
{
    clear();
    swap(__x);
    return *this;
}

would work just fine.  clear() is a no-op, and then you swap an empty  
capacity vector with itself.

My point is that I don't want to be told I need to put a if(this !=  
&x) in my move assignment operator.  It is a useless performance  
penalty.  Everyone should not have to pay (in terms of lost  
performance) for someone's bad code just to make sure self-move- 
assignment does a no-op instead of crashes.  If you want to cast an  
lvalue to an rvalue, that's fine.  Just make sure your code logic is  
such that such a cast is safe.  I believe std::swap's use of move  
assignment is safe because by the time it move-assigns, if it is self  
referencing, all of your resources have already been moved to a local  
temp.

-Howard