Re: [boost] [optional] problems with swap()

From: Andrey Semashev (andysem_at_[hidden])
Date: 2008-04-10 13:01:37

Thorsten Ottosen wrote:
> Hi Fernando,
> 
> I have some comments on the way optional<T>::swap() is implemented. I 
> looks like this:
> 
> // optional's swap:
> // If both are initialized, calls swap(T&, T&). If this swap throws, 
> //both will remain initialized but their values are now unspecified.
> // If only one is initialized, calls U.reset(*I), THEN I.reset().
> // If U.reset(*I) throws, both are left UNCHANGED (U is kept 
> //uinitialized and I is never reset)
> // If both are uninitialized, do nothing (no-throw)

[snip]

> The problem here is that the two first cases do not delegate to the 
> version of swap found by ADL. That only happens in cases where both 
> objects are initialized. This is, AFAICT, a major problem because 
> copy-construction has quite different semantics than swap() when it 
> comes to invalidation of iterators or pointers. It works ok for ints, 
> but creates havoc when using a container with internal memory.
> 
> I suggest we do something along these lines:

[snip]

> Comments?

Your approach requires the T to be default constructible which is not 
always the case. Besides, to my mind, the current implementation is more 
obvious for the user.

Your behavior can be implemented in a non-intrusive way something like that:

   template< typename T >
   void force_koenig_swap(optional< T >& left, optional< T >& right)
   {
     bool left_empty = !left;
     if (left_empty) left = in_place();
     bool right_empty = !right;
     if (right_empty) right = in_place();

     using std::swap;
     swap(left.get(), right.get());

     if (left_empty) right = none;
     if (right_empty) left = none;
   }

As for optional, I think, move construction would be more appropriate in 
case if one of the optionals is empty. And if T does not support moving 
the current implementation is good enough.