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]

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.voutilai...@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".