Re: [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
+  constexpr _Optional_payload(std::initializer_list<_Up> __il,
+ _Args&&... __args)
+   : _M_payload(__il, std::forward<_Args>(__args)...),
+ _M_engaged(true) {}
+
+  template  struct __ctor_tag {};


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


+  constexpr _Optional_payload(__ctor_tag,
+ const _Tp& __other)
+   : _M_payload(__other),
+ _M_engaged(true)
+  {}
+
+  constexpr _Optional_payload(__ctor_tag)
+   : _M_empty()
+  {}
+
+  constexpr _Optional_payload(__ctor_tag, _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{},
+ __other._M_payload) :
+   _Optional_payload(__ctor_tag{}))
+  {}
+
+  constexpr _Optional_payload(bool __engaged,
+ _Optional_payload&&


Nor here:


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


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{},
   std::move(__other._M_payload))
: _Optional_payload(__ctor_tag{}))



-  // Constructors for engaged optionals.
-  template, 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
+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
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.

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

[Ville Voutilainen]

Tested on Linux-x64.

2017-03-17  Ville Voutilainen  

Implement LWG 2900, The copy and move constructors
of optional are not constexpr.
* include/std/optional (_Optional_payload): New.
(_Optional_base): Remove the bool parameter.
(_Optional_base<_Tp, false>): Remove.
(_Optional_base()): Adjust.
(_Optional_base(nullopt_t)): Likewise.
(_Optional_base(in_place_t, _Args&&...)): Likewise.
(_Optional_base(in_place_t, initializer_list<_Up>, _Args&&...)):
Likewise.
(_Optional_base(const _Optional_base&)): Likewise.
(_Optional_base(_Optional_base&&)): Likewise.
(operator=(const _Optional_base&)): Likewise.
(operator=(_Optional_base&&)): Likewise.
(~_Optional_base()): Remove.
(_M_is_engaged()): Adjust.
(_M_get()): Likewise.
(_M_construct(_Args&&...)): Likewise.
(_M_destruct()): Likewise.
(_M_reset()): Likewise.
(_Optional_base::_Empty_byte): Remove.
(_Optional_base::_M_empty): Remove.
(_Optional_base::_M_payload): Adjust.
* testsuite/20_util/optional/cons/value_neg.cc: Adjust.
* testsuite/20_util/optional/constexpr/cons/value.cc: Add tests.
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
   __throw_bad_optional_access()
   { _GLIBCXX_THROW_OR_ABORT(bad_optional_access()); }
 
-  /**
-* @brief Class template that holds the necessary state for @ref optional
-* and that has the responsibility for construction and the special members.
-*
-* Such a separate base class template is necessary in order to
-* conditionally enable the special members (e.g. copy/move constructors).
-* Note that this means that @ref _Optional_base implements the
-* functionality for copy and move assignment, but not for converting
-* assignment.
-*
-* @see optional, _Enable_special_members
-*/
-  template::value>
-class _Optional_base
-{
-private:
-  // Remove const to avoid prohibition of reusing object storage for
-  // const-qualified types in [3.8/9]. This is strictly internal
-  // and even optional itself is oblivious to it.
-  using _Stored_type = remove_const_t<_Tp>;
 
-public:
+  // Payload for constexpr optionals.
+  template ::value
+ && is_trivially_move_constructible<_Tp>::value,
+   bool /*_ShouldProvideDestructor*/ =
+ is_trivially_destructible<_Tp>::value>
+struct _Optional_payload
+{
+  constexpr _Optional_payload()
+   : _M_empty() {}
 
-  // Constructors for disengaged optionals.
-  constexpr _Optional_base() noexcept
-  : _M_empty{} { }
+  template
+  constexpr _Optional_payload(in_place_t, _Args&&... __args)
+   : _M_payload(std::forward<_Args>(__args)...),
+ _M_engaged(true)
+  {}
 
-  constexpr _Optional_base(nullopt_t) noexcept
-  : _Optional_base{} { }
+  template
+  constexpr _Optional_payload(std::initializer_list<_Up> __il,
+ _Args&&... __args)
+   : _M_payload(__il, std::forward<_Args>(__args)...),
+ _M_engaged(true) {}
+
+  template  struct __ctor_tag {};
+  constexpr _Optional_payload(__ctor_tag,
+ const _Tp& __other)
+   : _M_payload(__other),
+ _M_engaged(true)
+  {}
+
+  constexpr _Optional_payload(__ctor_tag)
+   : _M_empty()
+  {}
+
+  constexpr _Optional_payload(__ctor_tag, _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{},
+ __other._M_payload) :
+   _Optional_payload(__ctor_tag{}))
+  {}
+
+  constexpr _Optional_payload(bool __engaged,
+ _Optional_payload&&
+ __other)
+   : _Optional_payload(__engaged ?
+   _Optional_payload(__ctor_tag{},
+ std::move(__other._M_payload)) :
+   _Optional_payload(__ctor_tag{}))
+  {}
 
-  // Constructors for engaged optionals.
-  template, 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 { };
+  union {
+  _Empty_byte _M_empty;
+  _Stored_type _M_payload;
+  };
+  bool _M_engaged = false;
+};
 
-