[v3 PATCH] Make optional conditionally trivially_{copy,move}_{constructible,assignable}
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 to a trivially copyable type. It's perhaps better to get that ABI break in now rather than later. So here you go (ho ho ho): 2017-12-25 Ville Voutilainen Make optional conditionally trivially_{copy,move}_{constructible,assignable} * include/std/optional (_Optional_payload): Fix the comment in the class head and turn into a primary and one specialization. (_Optional_payload::_M_engaged): Strike the NSDMI. (_Optional_payload<_Tp, false>::operator=(const _Optional_payload&)): New. (_Optional_payload<_Tp, false>::operator=(_Optional_payload&&)): Likewise. (_Optional_payload<_Tp, false>::_M_get): Likewise. (_Optional_payload<_Tp, false>::_M_reset): Likewise. (_Optional_base_impl): Likewise. (_Optional_base): Turn into a primary and three specializations. (optional(nullopt)): Change the base init. * testsuite/20_util/optional/assignment/8.cc: New. * testsuite/20_util/optional/cons/trivial.cc: Likewise. * testsuite/20_util/optional/cons/value_neg.cc: Adjust. diff --git a/libstdc++-v3/include/std/optional b/libstdc++-v3/include/std/optional index e017eed..9d1e625 100644 --- a/libstdc++-v3/include/std/optional +++ b/libstdc++-v3/include/std/optional @@ -100,15 +100,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION // Payload for constexpr optionals. template ::value - && is_trivially_move_constructible<_Tp>::value, - bool /*_ShouldProvideDestructor*/ = + bool /*_HasTrivialDestructor*/ = is_trivially_destructible<_Tp>::value> struct _Optional_payload { constexpr _Optional_payload() - : _M_empty() {} + : _M_empty(), _M_engaged(false) {} template constexpr _Optional_payload(in_place_t, _Args&&... __args) @@ -131,7 +128,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION {} constexpr _Optional_payload(__ctor_tag) - : _M_empty() + : _M_empty(), _M_engaged(false) {} constexpr _Optional_payload(__ctor_tag, _Tp&& __other) @@ -161,12 +158,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _Empty_byte _M_empty; _Stored_type _M_payload; }; - bool _M_engaged = false; + bool _M_engaged; }; - // Payload for non-constexpr optionals with non-trivial destructor. + // Payload for optionals with non-trivial destructor. template -struct _Optional_payload<_Tp, false, false> +struct _Optional_payload<_Tp, false> { constexpr _Optional_payload() : _M_empty() {} @@ -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_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 { @@ -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 -struct _Optional_payload<_Tp, false, true> -{ - constexpr _Optional_payload() - : _M_empty() {} - - template - constexpr _Optional_payload(in_place_t, _Args&&... __args) - : _M_payload(std::forward<_Args>(__args)...), - _M_engaged(true) {} - - template - 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 __engage
Re: [v3 PATCH] Make optional conditionally trivially_{copy,move}_{constructible,assignable}
On 8 January 2018 at 15:36, Jonathan Wakely wrote: > 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 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. New patch attached. I made _M_reset and _M_destruct noexcept, and added a comment about the protected inheritance in the code. Please double-check the whitespace department. diff --git a/libstdc++-v3/include/std/optional b/libstdc++-v3/include/std/optional index 466a11c..01fc06b 100644 --- a/libstdc++-v3/include/std/optional +++ b/libstdc++-v3/include/std/optional @@ -100,15 +100,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION // Payload for constexpr optionals. template ::value - && is_trivially_move_constructible<_Tp>::value, - bool /*_ShouldProvideDestructor*/ = + bool /*_HasTrivialDestructor*/ = is_trivially_destructible<_Tp>::value> struct _Optional_payload { constexpr _Optional_payload() - : _M_empty() {} + : _M_empty(), _M_engaged(false) {} template constexpr _Optional_payload(in_place_t, _Args&&... __args) @@ -131,7 +128,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION {} constexpr _Optional_payload(__ctor_tag) - : _M_empty() + : _M_empty(), _M_engaged(false) {} constexpr _Optional_payload(__ctor_tag, _Tp&& __other) @@ -161,12 +158,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _Empty_byte _M_empty; _Stored_type _M_payload; }; - bool _M_engaged = false; + bool _M_engaged; }; - // Payload for non-constexpr optionals with non-trivial destructor. + // Payload for optionals with non-trivial destructor. template -struct _Optional_payload<_Tp, false, false> +struct _Optional_payload<_Tp, false> { constexpr _Optional_payload() : _M_empty() {} @@ -203,6 +200,38 @@ _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_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 { @@ -226,95 +255,87 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _Stored_type(std::forward<_Args>(__args)...); this->_M_engaged = true; } -}; - // Payload for non-constexpr optionals with trivial destructor. - template -struct _Optional_payload<_Tp, false, true> -{ - constexpr _Optional_payload() - : _M_empty() {} - - template - constexpr _Optional_payload(in_place_t, _Args&&... __args) - : _M_payload(std::forward<_Args>(__args)...), - _M_engaged(true) {} - - template - 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) + // The _M_get operations have _M_engaged as a precondition. + constexpr _Tp& + _M_get() noexcept { - if (__other._M_engaged) - this->_M_construct(__other._M_payload); + return this->_M_payload; } - constexpr _Optional_payload(_Optional_payload&& __other) + constexpr const _Tp& + _M_get() const noexcept { - if (__other._M_engaged) -
Re: [v3 PATCH] Make optional conditionally trivially_{copy,move}_{constructible,assignable}
On 14/01/18 01:09 +0200, Ville Voutilainen wrote: On 8 January 2018 at 15:36, Jonathan Wakely wrote: 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 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. New patch attached. I made _M_reset and _M_destruct noexcept, and added a comment about the protected inheritance in the code. Please double-check the whitespace department. Thanks, OK for trunk.
Re: [v3 PATCH] Make optional conditionally trivially_{copy,move}_{constructible,assignable}
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 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_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 -struct _Optional_payload<_Tp, false, true> -{ - constexpr _Optional_payload() - : _M_empty() {} - - template - constexpr _Optional_payload(in_place_t, _Args&&... __args) - : _M_payload(std::forward<_Args>(__args)...), - _M_engaged(true) {} - - template - 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 +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 +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 -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? +