[boost] Re: [bug]: optional<T> fails on self-assignment

From: Fernando Cacciola (fernando_cacciola_at_[hidden])
Date: 2005-02-16 10:10:01

"David Abrahams" <dave_at_[hidden]> escribió en el mensaje 
news:u3bvwtvvq.fsf_at_boost-consulting.com...
> "Peter Dimov" <pdimov_at_[hidden]> writes:
>
>> David Abrahams wrote:
>>> "Joe Gottman" <jgottman_at_[hidden]> writes:
>>>
>>>>
>>>>    I disagree.  This may ensure the basic exception-safety
>>>> guarantee, but most code that uses optional<T> will use
>>>> T::operator=() at some point or other.  So if T::operator=() is not
>>>> exception-safe
>>>
>>> Whoa, here we go again!  You seem to be assuming
>>>
>>>  basic exception-safety guarantee == "not exception-safe."
>>>
>>> That's just not true for any reasonable definition of
>>> "exception-safe."
>>
>> No, he isn't assuming that. "Not exception safe" == "does not provide 
>> basic"
>> in the above paragraph. Fernando's goal, IIUC, was to make
>> optional<T>::operator= provide the basic guarantee even when T::operator=
>> does not. We are arguing that this is not necessary
>
> Agreed.  There's no good reason to try to "fix" broken classes.
>
>> and that optional<T>::operator= should just mirror the exception
>> safety guarantee of T::operator= and T::T( T const & ).
>
> Sounds fine to me.
>
Well, I'm kind of sure that this was discussed before when Boost.Optional 
was being reviewed and the conclusions were the opposite... but maybe I just 
got it wrong.
I have to admit that on a fresh look at it I can only agree with Peter here 
regarding the unwarranted additional guarantees from Optional.

That being said, I'm still unsure that the overhead of the swap idiom is 
worth it in this case were the swap isn't no-throw (that is, given that the 
idiom does not provide by itself any additional guarantees)

The correct fix along Joe's initial proposal would be:

void assign(optional_base const& rhs)
{
  if (is_initialized)
  {
    if ( rhs.is_initialized )
         get_impl() = rhs.get_impl();
    else destroy();
  }
  else
  {
    if ( rhs.is_initialized )
      construct(rhs.get_impl());
  }
}

AFAICS, this code is as safe aliasing-wise as it can be (it handles not only 
the trivial case of this==&rhs but also any other deeper aliasing issues)

If no voice is raised I'll proceed with this fix.

Best,

Fernando Cacciola