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 22:21 +0300, Ville Voutilainen wrote:

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


As discussed, destroying the value but leaving the manager non-null will
do bad things.


Oops again on my part! Not so carefully written, or tested.


New patch attached, ok for trunk?


OK, thanks.

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.

[Ville Voutilainen]

On 10 October 2016 at 21:19, Jonathan Wakely  wrote:
> 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.

Changed.

>> + 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.

Right, as discussed, this is now just a move assignment from a temporary.

>
>> }
>>   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

I removed the check, works fine.

> 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?

As discussed, destroying the value but leaving the manager non-null will
do bad things.

New patch attached, ok for trunk?

2016-10-10  Ville Voutilainen  

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() unconditionally in the case where
rhs has a value.
(operator=(_ValueType&&)): Indent the return type.
(_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..45a2145 100644
--- a/libstdc++-v3/include/std/any
+++ b/libstdc++-v3/include/std/any
@@ -179,6 +179,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
  typename _Tp = _Decay<_ValueType>,
  typename _Mgr = _Manager<_Tp>,
   __any_constructible_t<_Tp, _Args&&...> = false>
+  explicit
   any(in_place_type_t<_ValueType>, _Args&&... __args)
   : _M_manager(&_Mgr::_S_manage)
   {
@@ -192,6 +193,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
  typename _Mgr = _Manager<_Tp>,
   __any_constructible_t<_Tp, initializer_list<_Up>,
_Args&&...> = false>
+  explicit
   any(in_place_type_t<_ValueType>,
  initializer_list<_Up> __il, _Args&&... __args)
   : _M_manager(&_Mgr::_S_manage)
@@ -207,16 +209,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 /// Copy the state of another object.
 any& operator=(const any& __rhs)
 {
-  if (!__rhs.has_value())
-   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);
-   }
+  *this = any(__rhs);
   return *this;
 }
 
@@ -231,8 +224,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
reset();
   else if (this != &__rhs)
{
- if (has_value())
-   _M_manager(_Op_destroy, this, nullptr);
+ reset();
  _Arg __arg;
  __arg._M_any = this;
  __rhs._M_manager(_Op_xfer, &__rhs, &__arg);
@@ -243,7 +235,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 /// Store a copy of @p __rhs as the contained object.
 template>
-enable_if_t::value, any&>
+  enable_if_t::value, any&>
   operator=(_ValueType&& __rhs)
   {
*this = any(std::forward<_ValueType>(__rhs));
@@ -556,7 +548,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)));
__ptr->~_Tp();
__arg->_M_any->_M_manager = __any->_M_manager;
const_cast(__any)->_M_manager = nullptr;
diff --git a/libstdc++-v3/testsuite/20_util/any/assign/2.cc 
b/libstdc++-v3/testsuite/20_util/any/assign/2.cc
index b333e5d..28f06a0 100644
--- a/libstdc++-v3/testsuite/20_util/any/assign/2.cc
+++ 

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  

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

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 08/10/16 16:07 +0300, Ville Voutilainen wrote:

Tested on Linux-x64.

2016-10-08  Ville Voutilainen  

   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.

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

[Ville Voutilainen]

Tested on Linux-x64.

2016-10-08  Ville Voutilainen  

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)...);
@@ -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);
}
   return *this;
 }
@@ -232,7 +228,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   else if (this != &__rhs)
{
  if (has_value())
-   _M_manager(_Op_destroy, this, nullptr);
+   reset();
  _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)));
__ptr->~_Tp();
__arg->_M_any->_M_manager = __any->_M_manager;
const_cast(__any)->_M_manager = nullptr;
diff --git a/libstdc++-v3/testsuite/20_util/any/assign/2.cc 
b/libstdc++-v3/testsuite/20_util/any/assign/2.cc
index b333e5d..28f06a0 100644
--- a/libstdc++-v3/testsuite/20_util/any/assign/2.cc
+++ b/libstdc++-v3/testsuite/20_util/any/assign/2.cc
@@ -24,28 +24,69 @@
 using std::any;
 using std::any_cast;
 
+bool moved = false;
+bool copied = false;
+
 struct X
 {
-  bool moved = false;
-  bool moved_from = false;
   X() = default;
-  X(const X&) = default;
-  X(X&& x) : moved(true) { x.moved_from = true; }
+  X(const X&) { copied = true; }
+  X(X&& x) { moved = true; }
+};
+
+struct X2
+{
+  X2() = default;
+  X2(const X2&) { copied = true; }
+  X2(X2&& x) noexcept { moved = true; }
 };
 
 void test01()
 {
+  moved = false;
   X x;
   any a1;
   a1 = x;
-  VERIFY(x.moved_from == false);
+  VERIFY(moved == false);
   any a2;
+  copied = false;
   a2 = std::move(x);
-  VERIFY(x.moved_from == true);
-  VERIFY(any_cast(a2).moved == true );
+  VERIFY(moved == true);
+  VERIFY(copied == false);
+}
+
+void test02()
+{
+  moved = false;
+  X x;
+  any a1;
+  a1 = x;
+  VERIFY(moved == false);
+  any a2;
+  copied = false;
+  a2 = std::move(a1);
+  VERIFY(moved == false);
+  VERIFY(copied == false);
+}
+
+void test03()
+{
+  moved = false;
+  X2 x;
+  any a1;
+  a1 = x;
+  VERIFY(copied && moved);
+  any a2;
+  moved = false;
+  copied = false;
+  a2 = std::move(a1);
+  VERIFY(moved == true);
+  VERIFY(copied == false);
 }
 
 int main()
 {
   test01();
+  test02();
+  test03();
 }
diff --git a/libstdc++-v3/testsuite/20_util/any/assign/exception.cc 
b/libstdc++-v3/testsuite/20_util/any/assign/exception.cc
new file mode 100644
index 000..11a1a55
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/any/assign/exception.cc
@@ -0,0 +1,77 @@
+// { dg-options