[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 10/10/16 19:19 +0100, Jonathan Wakely wrote:
>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.

As discussed on IRC, it can be:

  else if (this != &__rhs)
    *this = any(__rhs);

which does one clone, one xfer and one destroy.

This way the effort of avoiding an extra xfer op is in the move
assignment operator.


As a drive-by fix, on operator=(_ValueType&& __rhs) please indent the
return type to line up with "operator".