Re: [v3 PATCH] Make any's copy assignment operator exception-safe, don't copy the underlying value when any is moved, make in_place constructors explicit.

[Jonathan Wakely <jwakely at redhat dot com>]

On 08/10/16 16:07 +0300, Ville Voutilainen wrote:
> Tested on Linux-x64.
>
> 2016-10-08  Ville Voutilainen  <ville.voutilainen@gmail.com>
>
>    Make any's copy assignment operator exception-safe,
>    don't copy the underlying value when any is moved,
>    make in_place constructors explicit.
>    * include/std/any (any(in_place_type_t<_ValueType>, _Args&&...)):
>    Make explicit.
>    (any(in_place_type_t<_ValueType>, initializer_list<_Up>, _Args&&...)):
>    Likewise.
>    (operator=(const any&)): Make strongly exception-safe.
>    (operator=(any&&)): Reset the manager when resetting the value.
>    This makes the state saner if an exception is thrown during the move.
>    (_Manager_internal<_Tp>::_S_manage): Move in _Op_xfer, don't copy.
>    * testsuite/20_util/any/assign/2.cc: Adjust.
>    * testsuite/20_util/any/assign/exception.cc: New.
>    * testsuite/20_util/any/cons/2.cc: Adjust.
>    * testsuite/20_util/any/cons/explicit.cc: New.
>    * testsuite/20_util/any/misc/any_cast_neg.cc: Ajust.

> diff --git a/libstdc++-v3/include/std/any b/libstdc++-v3/include/std/any
> index 9160035..78bdf89 100644
> --- a/libstdc++-v3/include/std/any
> +++ b/libstdc++-v3/include/std/any
> @@ -179,7 +179,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	      typename _Tp = _Decay<_ValueType>,
> 	      typename _Mgr = _Manager<_Tp>,
>               __any_constructible_t<_Tp, _Args&&...> = false>
> -      any(in_place_type_t<_ValueType>, _Args&&... __args)
> +      explicit any(in_place_type_t<_ValueType>, _Args&&... __args)
>       : _M_manager(&_Mgr::_S_manage)
>       {
>         _Mgr::_S_create(_M_storage, std::forward<_Args>(__args)...);
> @@ -192,8 +192,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	      typename _Mgr = _Manager<_Tp>,
>               __any_constructible_t<_Tp, initializer_list<_Up>,
> 				    _Args&&...> = false>
> -      any(in_place_type_t<_ValueType>,
> -	  initializer_list<_Up> __il, _Args&&... __args)
> +      explicit any(in_place_type_t<_ValueType>,
> +		   initializer_list<_Up> __il, _Args&&... __args)
>       : _M_manager(&_Mgr::_S_manage)
>       {
>         _Mgr::_S_create(_M_storage, __il, std::forward<_Args>(__args)...);

I prefer to put "explicit" on a line of its own, as we do for return
types, but I won't complain if you leave it like this.

> @@ -211,11 +211,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	reset();
>       else if (this != &__rhs)
> 	{
> -	  if (has_value())
> -	    _M_manager(_Op_destroy, this, nullptr);
> -	  _Arg __arg;
> -	  __arg._M_any = this;
> -	  __rhs._M_manager(_Op_clone, &__rhs, &__arg);
> +	  any(__rhs).swap(*this);

I was trying to avoid the "redundant" xfer operations that the swap
does, but I don't think we can do that and be exception safe. This is
simple and safe, and I think its optimal. Thanks.

> 	}
>       return *this;
>     }
> @@ -232,7 +228,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       else if (this != &__rhs)
> 	{
> 	  if (has_value())
> -	    _M_manager(_Op_destroy, this, nullptr);
> +	    reset();

If you're going to use reset() then you don't need the has_value()
check first. I think the reason I didn't use reset() was to avoid the
dead store to _M_manager that reset() does, since the compiler might
not detect it's dead (because the next store is done by the call
through a function pointer).

This code was all pretty carefully written to avoid any redundant
operations. Does this change buy us anything except simpler code?


> 	  _Arg __arg;
> 	  __arg._M_any = this;
> 	  __rhs._M_manager(_Op_xfer, &__rhs, &__arg);
> @@ -556,7 +552,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	__ptr->~_Tp();
> 	break;
>       case _Op_xfer:
> -	::new(&__arg->_M_any->_M_storage._M_buffer) _Tp(*__ptr);
> +	::new(&__arg->_M_any->_M_storage._M_buffer) _Tp
> +	  (std::move(*const_cast<_Tp*>(__ptr)));

I was looking at this recently and wondering why I did a copy not a
move. *cough* no redundant operations *cough* Oops.