[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]

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.