[v3 PATCH] Make optional conditionally trivially_{copy,move}_{constructible,assignable}

[Jonathan Wakely]

On 25/12/17 23:59 +0200, Ville Voutilainen wrote:
>In the midst of the holiday season, the king and ruler of all elves, otherwise
>known as The Elf, was told by little elves that users are complaining how
>stlstl and libc++ make optional's copy and move operations conditionally
>trivial, but libstdc++ doesn't. This made The Elf fairly angry, and he spoke
>"this will not stand".
>
>Tested on Linux-PPC64. The change is an ABI break due to changing
>optional<triviallycopyable> to a trivially copyable type. It's perhaps
>better to get that ABI break in now rather than later.

Agreed, but a few comments and questions below.


>@@ -203,6 +200,39 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	  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;
>+      }

Please make the whitespace before the return statement consistent in
these two assignment operators (one has a blank line and uses spaces,
one uses a tab).

>@@ -226,95 +256,86 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>             _Stored_type(std::forward<_Args>(__args)...);
>           this->_M_engaged = true;
>         }
>-    };
>-
>-  // Payload for non-constexpr optionals with trivial destructor.
>-  template <typename _Tp>
>-    struct _Optional_payload<_Tp, false, true>
>-    {
>-      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))
>-      {}
>+      // The _M_get operations have _M_engaged as a precondition.
>+      constexpr _Tp&
>+	_M_get() noexcept
>+      {
>+	return this->_M_payload;
>+      }
>
>-      constexpr _Optional_payload(const _Optional_payload& __other)
>+      constexpr const _Tp&
>+	_M_get() const noexcept
>       {
>-	if (__other._M_engaged)
>-	  this->_M_construct(__other._M_payload);
>+	return this->_M_payload;
>       }
>
>-      constexpr _Optional_payload(_Optional_payload&& __other)
>+      // _M_reset is a 'safe' operation with no precondition.
>+      void
>+      _M_reset()

Should this be noexcept?

>       {
>-	if (__other._M_engaged)
>-	  this->_M_construct(std::move(__other._M_payload));
>+	if (this->_M_engaged)
>+	  {
>+	    this->_M_engaged = false;
>+	    this->_M_payload.~_Stored_type();
>+	  }
>       }
>+  };

This closing brace seems to be indented incorrectly.

>-      using _Stored_type = remove_const_t<_Tp>;
>-      struct _Empty_byte { };
>-      union {
>-          _Empty_byte _M_empty;
>-          _Stored_type _M_payload;
>-      };
>-      bool _M_engaged = false;
>+  template<typename _Tp, typename _Dp>
>+    class _Optional_base_impl
>+  {
>+  protected:

And thos whole class body should be indented to line up with the
"class" keyword.

>+    using _Stored_type = remove_const_t<_Tp>;
>+
>+    // The _M_construct operation has !_M_engaged as a precondition
>+    // while _M_destruct has _M_engaged as a precondition.
>+    template<typename... _Args>
>+    void
>+    _M_construct(_Args&&... __args)
>+      noexcept(is_nothrow_constructible<_Stored_type, _Args...>())
>+    {
>+      ::new
>+	(std::__addressof(static_cast<_Dp*>(this)->_M_payload._M_payload))
>+	_Stored_type(std::forward<_Args>(__args)...);
>+      static_cast<_Dp*>(this)->_M_payload._M_engaged = true;
>+    }
>
>-      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;
>-        }
>-    };
>+    void
>+    _M_destruct()

noexcept?

>+    {
>+      static_cast<_Dp*>(this)->_M_payload._M_engaged = false;
>+      static_cast<_Dp*>(this)->_M_payload._M_payload.~_Stored_type();
>+    }
>+
>+    // _M_reset is a 'safe' operation with no precondition.
>+    void
>+    _M_reset()

noexcept?

>+  template<typename _Tp,
>+	   bool = is_trivially_copy_constructible_v<_Tp>,
>+	   bool = is_trivially_move_constructible_v<_Tp>>
>     class _Optional_base
>+      : protected _Optional_base_impl<_Tp, _Optional_base<_Tp>>

Is "protected" inheritance the right choice?

(Is protected inheritance *ever* the right choice?)

Everything in the base could be public, and then just use private
inheritance.


>+      // Copy and move constructors.
>+      constexpr _Optional_base(const _Optional_base& __other)
>+      = default;

Please put the line break after "constexpr" instead.

>+      // Copy and move constructors.
>+      constexpr _Optional_base(const _Optional_base& __other)
>+      = default;

Same here.


>diff --git a/libstdc++-v3/testsuite/20_util/optional/assignment/8.cc b/libstdc++-v3/testsuite/20_util/optional/assignment/8.cc
>new file mode 100644
>index 0000000..c00428e
>--- /dev/null
>+++ b/libstdc++-v3/testsuite/20_util/optional/assignment/8.cc
>@@ -0,0 +1,101 @@
>+// { dg-options "-std=gnu++17" }
>+// { dg-do compile }
>+
>+// Copyright (C) 2013-2017 Free Software Foundation, Inc.

>diff --git a/libstdc++-v3/testsuite/20_util/optional/cons/trivial.cc b/libstdc++-v3/testsuite/20_util/optional/cons/trivial.cc
>new file mode 100644
>index 0000000..541158e
>--- /dev/null
>+++ b/libstdc++-v3/testsuite/20_util/optional/cons/trivial.cc
>@@ -0,0 +1,47 @@
>+// { dg-options "-std=gnu++17" }
>+// { dg-do compile }
>+
>+// Copyright (C) 2013-2017 Free Software Foundation, Inc.

These new tests should just have a copyright date of 2018.