[v3] PR libstdc++/84601

Partially tested on Linux-x64, will run full suite on Linux-PPC64.

2018-02-28  Ville Voutilainen  <ville.voutilainen@gmail.com>

    PR libstdc++/84601
    * include/std/optional (_Optional_payload): Split into multiple
    specializations that can handle different cases of trivial or
    non-trivial assignment operators.
    * testsuite/20_util/optional/84601.cc: New.

On 28 February 2018 at 14:27, Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:
> Partially tested on Linux-x64, will run full suite on Linux-PPC64.

Which didn't reveal anything I didn't know, but reminded me to adjust
the negative tests. :)

2018-02-28  Ville Voutilainen  <ville.voutilainen@gmail.com>

    PR libstdc++/84601
    * include/std/optional (_Optional_payload): Split into multiple
    specializations that can handle different cases of trivial or
    non-trivial assignment operators.
    * testsuite/20_util/optional/84601.cc: New.
    * testsuite/20_util/optional/cons/value_neg.cc: Adjust.
diff --git a/libstdc++-v3/include/std/optional b/libstdc++-v3/include/std/optional
index 01fc06b..0aa20dd 100644
--- a/libstdc++-v3/include/std/optional
+++ b/libstdc++-v3/include/std/optional
@@ -98,13 +98,137 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   { _GLIBCXX_THROW_OR_ABORT(bad_optional_access()); }
 
 
-  // Payload for constexpr optionals.
+  // Payload for optionals with non-trivial destructor.
   template <typename _Tp,
 	    bool /*_HasTrivialDestructor*/ =
-	      is_trivially_destructible<_Tp>::value>
+	      is_trivially_destructible<_Tp>::value,
+	    bool /*_HasTrivialCopyAssignment*/ =
+	      is_trivially_copy_assignable<_Tp>::value,
+	    bool /*_HasTrivialMoveAssignment*/ =
+	      is_trivially_move_assignable<_Tp>::value>
     struct _Optional_payload
     {
       constexpr _Optional_payload()
+	: _M_empty() {}
+
+      template <typename... _Args>
+      constexpr _Optional_payload(in_place_t, _Args&&... __args)
+	: _M_payload(std::forward<_Args>(__args)...),
+	  _M_engaged(true) {}
+
+      template<typename _Up, typename... _Args>
+      constexpr _Optional_payload(std::initializer_list<_Up> __il,
+				  _Args&&... __args)
+	: _M_payload(__il, std::forward<_Args>(__args)...),
+	  _M_engaged(true) {}
+      constexpr
+      _Optional_payload(bool __engaged, const _Optional_payload& __other)
+	: _Optional_payload(__other)
+      {}
+
+      constexpr
+      _Optional_payload(bool __engaged, _Optional_payload&& __other)
+	: _Optional_payload(std::move(__other))
+      {}
+
+      constexpr _Optional_payload(const _Optional_payload& __other)
+      {
+	if (__other._M_engaged)
+	  this->_M_construct(__other._M_payload);
+      }
+
+      constexpr _Optional_payload(_Optional_payload&& __other)
+      {
+	if (__other._M_engaged)
+	  this->_M_construct(std::move(__other._M_payload));
+      }
+
+      _Optional_payload&
+      operator=(const _Optional_payload& __other)
+      {
+        if (this->_M_engaged && __other._M_engaged)
+          this->_M_get() = __other._M_get();
+        else
+	  {
+	    if (__other._M_engaged)
+	      this->_M_construct(__other._M_get());
+	    else
+	      this->_M_reset();
+	  }
+	return *this;
+      }
+
+      _Optional_payload&
+      operator=(_Optional_payload&& __other)
+      noexcept(__and_<is_nothrow_move_constructible<_Tp>,
+		      is_nothrow_move_assignable<_Tp>>())
+      {
+	if (this->_M_engaged && __other._M_engaged)
+	  this->_M_get() = std::move(__other._M_get());
+	else
+	  {
+	    if (__other._M_engaged)
+	      this->_M_construct(std::move(__other._M_get()));
+	    else
+	      this->_M_reset();
+	  }
+	return *this;
+      }
+
+      using _Stored_type = remove_const_t<_Tp>;
+      struct _Empty_byte { };
+      union {
+          _Empty_byte _M_empty;
+          _Stored_type _M_payload;
+      };
+      bool _M_engaged = false;
+
+      ~_Optional_payload()
+      {
+        if (_M_engaged)
+          _M_payload.~_Stored_type();
+      }
+
+      template<typename... _Args>
+        void
+        _M_construct(_Args&&... __args)
+        noexcept(is_nothrow_constructible<_Stored_type, _Args...>())
+        {
+          ::new ((void *) std::__addressof(this->_M_payload))
+            _Stored_type(std::forward<_Args>(__args)...);
+          this->_M_engaged = true;
+        }
+
+      // The _M_get operations have _M_engaged as a precondition.
+      constexpr _Tp&
+	_M_get() noexcept
+      {
+	return this->_M_payload;
+      }
+
+      constexpr const _Tp&
+	_M_get() const noexcept
+      {
+	return this->_M_payload;
+      }
+
+      // _M_reset is a 'safe' operation with no precondition.
+      void
+      _M_reset() noexcept
+      {
+	if (this->_M_engaged)
+	  {
+	    this->_M_engaged = false;
+	    this->_M_payload.~_Stored_type();
+	  }
+      }
+    };
+
+  // Payload for constexpr optionals.
+  template <typename _Tp>
+    struct _Optional_payload<_Tp, true, true, true>
+    {
+      constexpr _Optional_payload()
 	: _M_empty(), _M_engaged(false) {}
 
       template<typename... _Args>
@@ -161,44 +285,294 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       bool _M_engaged;
     };
 
-  // Payload for optionals with non-trivial destructor.
+  // Payload for optionals with non-trivial copy assignment.
   template <typename _Tp>
-    struct _Optional_payload<_Tp, false>
+    struct _Optional_payload<_Tp, true, false, true>
     {
       constexpr _Optional_payload()
-	: _M_empty() {}
+	: _M_empty(), _M_engaged(false) {}
 
-      template <typename... _Args>
+      template<typename... _Args>
       constexpr _Optional_payload(in_place_t, _Args&&... __args)
 	: _M_payload(std::forward<_Args>(__args)...),
+	  _M_engaged(true)
+      {}
+
+      template<typename _Up, typename... _Args>
+      constexpr _Optional_payload(std::initializer_list<_Up> __il,
+				  _Args&&... __args)
+	: _M_payload(__il, std::forward<_Args>(__args)...),
 	  _M_engaged(true) {}
 
+      template <class _Up> struct __ctor_tag {};
+
+      constexpr _Optional_payload(__ctor_tag<bool>,
+				  const _Tp& __other)
+	: _M_payload(__other),
+	  _M_engaged(true)
+      {}
+
+      constexpr _Optional_payload(__ctor_tag<void>)
+	: _M_empty(), _M_engaged(false)
+      {}
+
+      constexpr _Optional_payload(__ctor_tag<bool>, _Tp&& __other)
+	: _M_payload(std::move(__other)),
+	  _M_engaged(true)
+      {}
+
+      constexpr _Optional_payload(bool __engaged,
+				  const _Optional_payload& __other)
+	: _Optional_payload(__engaged ?
+			    _Optional_payload(__ctor_tag<bool>{},
+					      __other._M_payload) :
+			    _Optional_payload(__ctor_tag<void>{}))
+      {}
+
+      constexpr _Optional_payload(bool __engaged,
+				  _Optional_payload&& __other)
+	: _Optional_payload(__engaged
+			    ? _Optional_payload(__ctor_tag<bool>{},
+						std::move(__other._M_payload))
+			    : _Optional_payload(__ctor_tag<void>{}))
+      {}
+
+      _Optional_payload(const _Optional_payload&) = default;
+      _Optional_payload(_Optional_payload&&) = default;
+
+      _Optional_payload&
+      operator=(const _Optional_payload& __other)
+      {
+        if (this->_M_engaged && __other._M_engaged)
+          this->_M_get() = __other._M_get();
+        else
+	  {
+	    if (__other._M_engaged)
+	      this->_M_construct(__other._M_get());
+	    else
+	      this->_M_reset();
+	  }
+	return *this;
+      }
+
+      _Optional_payload&
+      operator=(_Optional_payload&& __other) = default;
+
+      using _Stored_type = remove_const_t<_Tp>;
+      struct _Empty_byte { };
+      union {
+          _Empty_byte _M_empty;
+          _Stored_type _M_payload;
+      };
+      bool _M_engaged;
+
+      template<typename... _Args>
+        void
+        _M_construct(_Args&&... __args)
+        noexcept(is_nothrow_constructible<_Stored_type, _Args...>())
+        {
+          ::new ((void *) std::__addressof(this->_M_payload))
+            _Stored_type(std::forward<_Args>(__args)...);
+          this->_M_engaged = true;
+        }
+
+      // The _M_get operations have _M_engaged as a precondition.
+      constexpr _Tp&
+	_M_get() noexcept
+      {
+	return this->_M_payload;
+      }
+
+      constexpr const _Tp&
+	_M_get() const noexcept
+      {
+	return this->_M_payload;
+      }
+
+      // _M_reset is a 'safe' operation with no precondition.
+      void
+      _M_reset() noexcept
+      {
+	if (this->_M_engaged)
+	  {
+	    this->_M_engaged = false;
+	    this->_M_payload.~_Stored_type();
+	  }
+      }
+    };
+
+  // Payload for optionals with non-trivial move assignment.
+  template <typename _Tp>
+    struct _Optional_payload<_Tp, true, true, false>
+    {
+      constexpr _Optional_payload()
+	: _M_empty(), _M_engaged(false) {}
+
+      template<typename... _Args>
+      constexpr _Optional_payload(in_place_t, _Args&&... __args)
+	: _M_payload(std::forward<_Args>(__args)...),
+	  _M_engaged(true)
+      {}
+
       template<typename _Up, typename... _Args>
       constexpr _Optional_payload(std::initializer_list<_Up> __il,
 				  _Args&&... __args)
 	: _M_payload(__il, std::forward<_Args>(__args)...),
 	  _M_engaged(true) {}
-      constexpr
-      _Optional_payload(bool __engaged, const _Optional_payload& __other)
-	: _Optional_payload(__other)
+
+      template <class _Up> struct __ctor_tag {};
+
+      constexpr _Optional_payload(__ctor_tag<bool>,
+				  const _Tp& __other)
+	: _M_payload(__other),
+	  _M_engaged(true)
       {}
 
-      constexpr
-      _Optional_payload(bool __engaged, _Optional_payload&& __other)
-	: _Optional_payload(std::move(__other))
+      constexpr _Optional_payload(__ctor_tag<void>)
+	: _M_empty(), _M_engaged(false)
       {}
 
-      constexpr _Optional_payload(const _Optional_payload& __other)
+      constexpr _Optional_payload(__ctor_tag<bool>, _Tp&& __other)
+	: _M_payload(std::move(__other)),
+	  _M_engaged(true)
+      {}
+
+      constexpr _Optional_payload(bool __engaged,
+				  const _Optional_payload& __other)
+	: _Optional_payload(__engaged ?
+			    _Optional_payload(__ctor_tag<bool>{},
+					      __other._M_payload) :
+			    _Optional_payload(__ctor_tag<void>{}))
+      {}
+
+      constexpr _Optional_payload(bool __engaged,
+				  _Optional_payload&& __other)
+	: _Optional_payload(__engaged
+			    ? _Optional_payload(__ctor_tag<bool>{},
+						std::move(__other._M_payload))
+			    : _Optional_payload(__ctor_tag<void>{}))
+      {}
+
+      _Optional_payload(const _Optional_payload&) = default;
+      _Optional_payload(_Optional_payload&&) = default;
+
+      _Optional_payload&
+      operator=(const _Optional_payload& __other) = default;
+
+      _Optional_payload&
+      operator=(_Optional_payload&& __other)
+      noexcept(__and_<is_nothrow_move_constructible<_Tp>,
+		      is_nothrow_move_assignable<_Tp>>())
       {
-	if (__other._M_engaged)
-	  this->_M_construct(__other._M_payload);
+	if (this->_M_engaged && __other._M_engaged)
+	  this->_M_get() = std::move(__other._M_get());
+	else
+	  {
+	    if (__other._M_engaged)
+	      this->_M_construct(std::move(__other._M_get()));
+	    else
+	      this->_M_reset();
+	  }
+	return *this;
       }
 
-      constexpr _Optional_payload(_Optional_payload&& __other)
+      using _Stored_type = remove_const_t<_Tp>;
+      struct _Empty_byte { };
+      union {
+          _Empty_byte _M_empty;
+          _Stored_type _M_payload;
+      };
+      bool _M_engaged;
+
+      template<typename... _Args>
+        void
+        _M_construct(_Args&&... __args)
+        noexcept(is_nothrow_constructible<_Stored_type, _Args...>())
+        {
+          ::new ((void *) std::__addressof(this->_M_payload))
+            _Stored_type(std::forward<_Args>(__args)...);
+          this->_M_engaged = true;
+        }
+
+      // The _M_get operations have _M_engaged as a precondition.
+      constexpr _Tp&
+	_M_get() noexcept
       {
-	if (__other._M_engaged)
-	  this->_M_construct(std::move(__other._M_payload));
+	return this->_M_payload;
+      }
+
+      constexpr const _Tp&
+	_M_get() const noexcept
+      {
+	return this->_M_payload;
+      }
+
+      // _M_reset is a 'safe' operation with no precondition.
+      void
+      _M_reset() noexcept
+      {
+	if (this->_M_engaged)
+	  {
+	    this->_M_engaged = false;
+	    this->_M_payload.~_Stored_type();
+	  }
       }
+    };
+
+  // Payload for optionals with non-trivial copy and move assignment.
+  template <typename _Tp>
+    struct _Optional_payload<_Tp, true, false, false>
+    {
+      constexpr _Optional_payload()
+	: _M_empty(), _M_engaged(false) {}
+
+      template<typename... _Args>
+      constexpr _Optional_payload(in_place_t, _Args&&... __args)
+	: _M_payload(std::forward<_Args>(__args)...),
+	  _M_engaged(true)
+      {}
+
+      template<typename _Up, typename... _Args>
+      constexpr _Optional_payload(std::initializer_list<_Up> __il,
+				  _Args&&... __args)
+	: _M_payload(__il, std::forward<_Args>(__args)...),
+	  _M_engaged(true) {}
+
+      template <class _Up> struct __ctor_tag {};
+
+      constexpr _Optional_payload(__ctor_tag<bool>,
+				  const _Tp& __other)
+	: _M_payload(__other),
+	  _M_engaged(true)
+      {}
+
+      constexpr _Optional_payload(__ctor_tag<void>)
+	: _M_empty(), _M_engaged(false)
+      {}
+
+      constexpr _Optional_payload(__ctor_tag<bool>, _Tp&& __other)
+	: _M_payload(std::move(__other)),
+	  _M_engaged(true)
+      {}
+
+      constexpr _Optional_payload(bool __engaged,
+				  const _Optional_payload& __other)
+	: _Optional_payload(__engaged ?
+			    _Optional_payload(__ctor_tag<bool>{},
+					      __other._M_payload) :
+			    _Optional_payload(__ctor_tag<void>{}))
+      {}
+
+      constexpr _Optional_payload(bool __engaged,
+				  _Optional_payload&& __other)
+	: _Optional_payload(__engaged
+			    ? _Optional_payload(__ctor_tag<bool>{},
+						std::move(__other._M_payload))
+			    : _Optional_payload(__ctor_tag<void>{}))
+      {}
+
+      _Optional_payload(const _Optional_payload&) = default;
+      _Optional_payload(_Optional_payload&&) = default;
 
       _Optional_payload&
       operator=(const _Optional_payload& __other)
@@ -238,13 +612,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
           _Empty_byte _M_empty;
           _Stored_type _M_payload;
       };
-      bool _M_engaged = false;
-
-      ~_Optional_payload()
-      {
-        if (_M_engaged)
-          _M_payload.~_Stored_type();
-      }
+      bool _M_engaged;
 
       template<typename... _Args>
         void
@@ -280,7 +648,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  }
       }
     };
-  
+
   template<typename _Tp, typename _Dp>
     class _Optional_base_impl
     {
diff --git a/libstdc++-v3/testsuite/20_util/optional/84601.cc b/libstdc++-v3/testsuite/20_util/optional/84601.cc
new file mode 100644
index 0000000..e86d39e
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/optional/84601.cc
@@ -0,0 +1,22 @@
+// { dg-options "-std=gnu++17" }
+// { dg-do compile }
+
+#include <optional>
+
+using pair_t = std::pair<int, int>;
+using opt_t = std::optional<pair_t>;
+
+static_assert(std::is_copy_constructible_v<opt_t::value_type>);
+static_assert(std::is_copy_assignable_v<opt_t::value_type>);
+
+static_assert(std::is_copy_assignable_v<opt_t>); // assertion fails.
+
+class A
+{
+  void f(const opt_t& opt)
+  {
+    _opt = opt;
+  }
+
+  opt_t _opt;
+};
diff --git a/libstdc++-v3/testsuite/20_util/optional/cons/value_neg.cc b/libstdc++-v3/testsuite/20_util/optional/cons/value_neg.cc
index c448996..ae55ab2 100644
--- a/libstdc++-v3/testsuite/20_util/optional/cons/value_neg.cc
+++ b/libstdc++-v3/testsuite/20_util/optional/cons/value_neg.cc
@@ -37,8 +37,8 @@ int main()
     std::optional<std::unique_ptr<int>> oup2 = new int;  // { dg-error "conversion" }
     struct U { explicit U(std::in_place_t); };
     std::optional<U> ou(std::in_place); // { dg-error "no matching" }
-    // { dg-error "no type" "" { target { *-*-* } } 647 }
-    // { dg-error "no type" "" { target { *-*-* } } 657 }
-    // { dg-error "no type" "" { target { *-*-* } } 714 }
+    // { dg-error "no type" "" { target { *-*-* } } 1015 }
+    // { dg-error "no type" "" { target { *-*-* } } 1025 }
+    // { dg-error "no type" "" { target { *-*-* } } 1082 }
   }
 }

On 28/02/18 15:12 +0200, Ville Voutilainen wrote:
>-  // Payload for constexpr optionals.
>+  // Payload for optionals with non-trivial destructor.
>   template <typename _Tp,
>           bool /*_HasTrivialDestructor*/ =
>-            is_trivially_destructible<_Tp>::value>
>+            is_trivially_destructible<_Tp>::value,
>+          bool /*_HasTrivialCopyAssignment*/ =
>+            is_trivially_copy_assignable<_Tp>::value,
>+          bool /*_HasTrivialMoveAssignment*/ =
>+            is_trivially_move_assignable<_Tp>::value>

I'm not sure these comments are very useful, as they just repeat the
info that the traits already give us. Also, you could use the _v
variable templates if you wanted (doesn't make much difference
though).

But on the subject of redundant comments ...

>     struct _Optional_payload

It took me a minute to figure out which conditions the primary
template gets used for, to double-check the comment. Would it be
helpful to use a comment like:

     struct _Optional_payload // <false, _TrivialCopy, _TrivialMove>

or does that not really clarify anything?

I suppose it doesn't tell us any more than the "non-trivial
destructor" comment you already have.

So OK for trunk then.