[v3 PATCH] Implement LWG 2900, The copy and move constructors of optional are not constexpr.

[Jonathan Wakely]

On 17/03/17 16:51 +0200, Ville Voutilainen wrote:
>diff --git a/libstdc++-v3/include/std/optional b/libstdc++-v3/include/std/optional
>index 3f540ec..e67ba89 100644
>--- a/libstdc++-v3/include/std/optional
>+++ b/libstdc++-v3/include/std/optional
>@@ -95,125 +95,127 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>-      constexpr _Optional_base(nullopt_t) noexcept
>-      : _Optional_base{} { }
>+      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 {};

Newline here please, so it doesn't look like this template<...> is on
the constructor.

>+      constexpr _Optional_payload(__ctor_tag<bool>,
>+				  const _Tp& __other)
>+	: _M_payload(__other),
>+	  _M_engaged(true)
>+      {}
>+
>+      constexpr _Optional_payload(__ctor_tag<void>)
>+	: _M_empty()
>+      {}
>+
>+      constexpr _Optional_payload(__ctor_tag<bool>, _Tp&& __other)
>+	: _M_payload(std::move(__other)),
>+	  _M_engaged(true)
>+      {}
>+
>+      constexpr _Optional_payload(bool __engaged,
>+				  const _Optional_payload&

Looks like we don't need a newline before the parameter name here:

>+				  __other)
>+	: _Optional_payload(__engaged ?
>+			    _Optional_payload(__ctor_tag<bool>{},
>+					      __other._M_payload) :
>+			    _Optional_payload(__ctor_tag<void>{}))
>+      {}
>+
>+      constexpr _Optional_payload(bool __engaged,
>+				  _Optional_payload&&

Nor here:

>+				  __other)
>+	: _Optional_payload(__engaged ?
>+			    _Optional_payload(__ctor_tag<bool>{},
>+					      std::move(__other._M_payload)) :
>+			    _Optional_payload(__ctor_tag<void>{}))
>+      {}

Hmm, I see we don't actually show a conditional expression in the
Coding Style docs, but I'd do that as:

	: _Optional_payload(__engaged
			    ? _Optional_payload(__ctor_tag<bool>{},
                                                std::move(__other._M_payload))
			    : _Optional_payload(__ctor_tag<void>{}))


>-      // Constructors for engaged optionals.
>-      template<typename... _Args,
>-	       enable_if_t<is_constructible_v<_Tp, _Args&&...>, bool> = false>
>-        constexpr explicit _Optional_base(in_place_t, _Args&&... __args)
>-        : _M_payload(std::forward<_Args>(__args)...), _M_engaged(true) { }
>+      using _Stored_type = remove_const_t<_Tp>;
>+      struct _Empty_byte { };

I was going to ask whether std::byte would be better here, but I think
a valueless struct is better.

>+      union {
>+          _Empty_byte _M_empty;
>+          _Stored_type _M_payload;
>+      };
>+      bool _M_engaged = false;
>+    };


>+      template<typename... _Args>
>+        void
>+        _M_construct(_Args&&... __args)
>+        noexcept(is_nothrow_constructible<_Stored_type, _Args...>())
>+        {
>+          ::new (std::__addressof(this->_M_payload))

I think we need (void*) here to ensure we use the reserved placement
form of operator new and not some other overload:

struct X { };
void* operator new(decltype(sizeof(0)) n, X*);

(We don't get this right everywhere that uses placement new with
arbitrary types).


>+            _Stored_type(std::forward<_Args>(__args)...);
>+          this->_M_engaged = true;
>+        }
>     };
>
>+      // 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(this->_M_payload))
>+          ::new (std::__addressof(this->_M_payload._M_payload))

Here too.

>             _Stored_type(std::forward<_Args>(__args)...);
>-          this->_M_engaged = true;
>+          this->_M_payload._M_engaged = true;
>         }

OK for trunk with those adjustements, thanks.