| Message ID | CAFk2RUa3nPu-iBceNtn_GctBf5Ht6y1kK0bUoMbmu-oA8Tsg8g@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] Make optional conditionally trivially_{copy,move}_{constructible,assignable} | expand |
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.
On 8 January 2018 at 15:36, Jonathan Wakely <jwakely@redhat.com> 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<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. 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 <typename _Tp, - bool /*_TrivialCopyMove*/ = - is_trivially_copy_constructible<_Tp>::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<typename... _Args> constexpr _Optional_payload(in_place_t, _Args&&... __args) @@ -131,7 +128,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION {} constexpr _Optional_payload(__ctor_tag<void>) - : _M_empty() + : _M_empty(), _M_engaged(false) {} constexpr _Optional_payload(__ctor_tag<bool>, _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 <typename _Tp> - 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_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 { @@ -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 <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)) - {} - - 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) - this->_M_construct(std::move(__other._M_payload)); + 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(); + } + } + }; + + template<typename _Tp, typename _Dp> + class _Optional_base_impl + { + protected: using _Stored_type = remove_const_t<_Tp>; - struct _Empty_byte { }; - union { - _Empty_byte _M_empty; - _Stored_type _M_payload; - }; - bool _M_engaged = false; - + + // 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 ((void *) std::__addressof(this->_M_payload)) - _Stored_type(std::forward<_Args>(__args)...); - this->_M_engaged = true; - } - }; + 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; + } + + 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 + { + if (static_cast<_Dp*>(this)->_M_payload._M_engaged) + static_cast<_Dp*>(this)->_M_destruct(); + } + }; /** - * @brief Class template that holds the necessary state for @ref optional - * and that has the responsibility for construction and the special members. + * @brief Class template that takes care of copy/move constructors + of optional * * 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. - * + * conditionally make copy/move constructors trivial. * @see optional, _Enable_special_members */ - template<typename _Tp> + template<typename _Tp, + bool = is_trivially_copy_constructible_v<_Tp>, + bool = is_trivially_move_constructible_v<_Tp>> class _Optional_base + // protected inheritance because optional needs to reach that base too + : protected _Optional_base_impl<_Tp, _Optional_base<_Tp>> { - 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>; - + friend class _Optional_base_impl<_Tp, _Optional_base<_Tp>>; public: // Constructors for disengaged optionals. - constexpr _Optional_base() noexcept - { } - - constexpr _Optional_base(nullopt_t) noexcept - { } + constexpr _Optional_base() = default; // Constructors for engaged optionals. template<typename... _Args, @@ -347,84 +368,216 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { } // Assignment operators. - _Optional_base& - operator=(const _Optional_base& __other) + _Optional_base& operator=(const _Optional_base&) = default; + _Optional_base& operator=(_Optional_base&&) = default; + + protected: + + constexpr bool _M_is_engaged() const noexcept + { return this->_M_payload._M_engaged; } + + // The _M_get operations have _M_engaged as a precondition. + constexpr _Tp& + _M_get() noexcept { - if (this->_M_payload._M_engaged && __other._M_payload._M_engaged) - this->_M_get() = __other._M_get(); - else - { - if (__other._M_payload._M_engaged) - this->_M_construct(__other._M_get()); - else - this->_M_reset(); - } + __glibcxx_assert(this->_M_is_engaged()); + return this->_M_payload._M_payload; + } - return *this; + constexpr const _Tp& + _M_get() const noexcept + { + __glibcxx_assert(this->_M_is_engaged()); + return this->_M_payload._M_payload; } - _Optional_base& - operator=(_Optional_base&& __other) - noexcept(__and_<is_nothrow_move_constructible<_Tp>, - is_nothrow_move_assignable<_Tp>>()) + private: + _Optional_payload<_Tp> _M_payload; + }; + + template<typename _Tp> + class _Optional_base<_Tp, false, true> + : protected _Optional_base_impl<_Tp, _Optional_base<_Tp>> + { + friend class _Optional_base_impl<_Tp, _Optional_base<_Tp>>; + public: + + // Constructors for disengaged optionals. + constexpr _Optional_base() = default; + + // 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(in_place, + std::forward<_Args>(__args)...) { } + + template<typename _Up, typename... _Args, + enable_if_t<is_constructible_v<_Tp, + initializer_list<_Up>&, + _Args&&...>, bool> = false> + constexpr explicit _Optional_base(in_place_t, + initializer_list<_Up> __il, + _Args&&... __args) + : _M_payload(in_place, + __il, std::forward<_Args>(__args)...) + { } + + // Copy and move constructors. + constexpr _Optional_base(const _Optional_base& __other) + : _M_payload(__other._M_payload._M_engaged, + __other._M_payload) + { } + + constexpr _Optional_base(_Optional_base&& __other) = default; + + // Assignment operators. + _Optional_base& operator=(const _Optional_base&) = default; + _Optional_base& operator=(_Optional_base&&) = default; + + protected: + + constexpr bool _M_is_engaged() const noexcept + { return this->_M_payload._M_engaged; } + + // The _M_get operations have _M_engaged as a precondition. + constexpr _Tp& + _M_get() noexcept { - if (this->_M_payload._M_engaged && __other._M_payload._M_engaged) - this->_M_get() = std::move(__other._M_get()); - else - { - if (__other._M_payload._M_engaged) - this->_M_construct(std::move(__other._M_get())); - else - this->_M_reset(); - } - return *this; + __glibcxx_assert(this->_M_is_engaged()); + return this->_M_payload._M_payload; + } + + constexpr const _Tp& + _M_get() const noexcept + { + __glibcxx_assert(this->_M_is_engaged()); + return this->_M_payload._M_payload; } - // The following functionality is also needed by optional, hence the - // protected accessibility. + + private: + _Optional_payload<_Tp> _M_payload; + }; + + template<typename _Tp> + class _Optional_base<_Tp, true, false> + : protected _Optional_base_impl<_Tp, _Optional_base<_Tp>> + { + friend class _Optional_base_impl<_Tp, _Optional_base<_Tp>>; + public: + + // Constructors for disengaged optionals. + constexpr _Optional_base() = default; + + // 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(in_place, + std::forward<_Args>(__args)...) { } + + template<typename _Up, typename... _Args, + enable_if_t<is_constructible_v<_Tp, + initializer_list<_Up>&, + _Args&&...>, bool> = false> + constexpr explicit _Optional_base(in_place_t, + initializer_list<_Up> __il, + _Args&&... __args) + : _M_payload(in_place, + __il, std::forward<_Args>(__args)...) + { } + + // Copy and move constructors. + constexpr _Optional_base(const _Optional_base& __other) = default; + + constexpr _Optional_base(_Optional_base&& __other) + noexcept(is_nothrow_move_constructible<_Tp>()) + : _M_payload(__other._M_payload._M_engaged, + std::move(__other._M_payload)) + { } + + // Assignment operators. + _Optional_base& operator=(const _Optional_base&) = default; + _Optional_base& operator=(_Optional_base&&) = default; + protected: + constexpr bool _M_is_engaged() const noexcept { return this->_M_payload._M_engaged; } // The _M_get operations have _M_engaged as a precondition. constexpr _Tp& - _M_get() noexcept + _M_get() noexcept { - __glibcxx_assert(_M_is_engaged()); + __glibcxx_assert(this->_M_is_engaged()); return this->_M_payload._M_payload; } constexpr const _Tp& - _M_get() const noexcept + _M_get() const noexcept { - __glibcxx_assert(_M_is_engaged()); + __glibcxx_assert(this->_M_is_engaged()); return this->_M_payload._M_payload; } - // 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._M_payload)) - _Stored_type(std::forward<_Args>(__args)...); - this->_M_payload._M_engaged = true; - } + private: + _Optional_payload<_Tp> _M_payload; + }; - void - _M_destruct() + template<typename _Tp> + class _Optional_base<_Tp, true, true> + : protected _Optional_base_impl<_Tp, _Optional_base<_Tp>> + { + friend class _Optional_base_impl<_Tp, _Optional_base<_Tp>>; + public: + + // Constructors for disengaged optionals. + constexpr _Optional_base() = default; + + // 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(in_place, + std::forward<_Args>(__args)...) { } + + template<typename _Up, typename... _Args, + enable_if_t<is_constructible_v<_Tp, + initializer_list<_Up>&, + _Args&&...>, bool> = false> + constexpr explicit _Optional_base(in_place_t, + initializer_list<_Up> __il, + _Args&&... __args) + : _M_payload(in_place, + __il, std::forward<_Args>(__args)...) + { } + + // Copy and move constructors. + constexpr _Optional_base(const _Optional_base& __other) = default; + constexpr _Optional_base(_Optional_base&& __other) = default; + + // Assignment operators. + _Optional_base& operator=(const _Optional_base&) = default; + _Optional_base& operator=(_Optional_base&&) = default; + + protected: + + constexpr bool _M_is_engaged() const noexcept + { return this->_M_payload._M_engaged; } + + // The _M_get operations have _M_engaged as a precondition. + constexpr _Tp& + _M_get() noexcept { - this->_M_payload._M_engaged = false; - this->_M_payload._M_payload.~_Stored_type(); + __glibcxx_assert(this->_M_is_engaged()); + return this->_M_payload._M_payload; } - // _M_reset is a 'safe' operation with no precondition. - void - _M_reset() + constexpr const _Tp& + _M_get() const noexcept { - if (this->_M_payload._M_engaged) - this->_M_destruct(); + __glibcxx_assert(this->_M_is_engaged()); + return this->_M_payload._M_payload; } private: @@ -482,8 +635,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION constexpr optional() = default; - constexpr optional(nullopt_t) noexcept - : _Base(nullopt) { } + constexpr optional(nullopt_t) noexcept { } // Converting constructors for engaged optionals. template <typename _Up = _Tp, 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..d573137 --- /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) 2018 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +#include <optional> + +struct X +{ + ~X(); +}; + +struct Y +{ + Y& operator=(const Y&) = default; + Y& operator=(Y&&); + Y(const Y&) = default; + Y(Y&&) = default; +}; + +struct Z +{ + Z& operator=(const Z&); + Z& operator=(Z&&) = default; + Z(const Z&) = default; +}; + +struct Y2 +{ + Y2& operator=(const Y2&) = default; + Y2& operator=(Y2&&); +}; + +struct Z2 +{ + Z2& operator=(const Z2&); + Z2& operator=(Z2&&) = default; +}; + +static_assert(std::is_trivially_copy_assignable_v<std::optional<int>>); +static_assert(std::is_trivially_move_assignable_v<std::optional<int>>); +static_assert(!std::is_trivially_copy_assignable_v<std::optional<X>>); +static_assert(!std::is_trivially_move_assignable_v<std::optional<X>>); +static_assert(std::is_trivially_copy_assignable_v<std::optional<Y>>); +static_assert(!std::is_trivially_move_assignable_v<std::optional<Y>>); +static_assert(!std::is_trivially_copy_assignable_v<std::optional<Z>>); +static_assert(std::is_trivially_move_assignable_v<std::optional<Z>>); +static_assert(std::is_trivially_copy_assignable_v<Y2>); +static_assert(!std::is_trivially_move_assignable_v<Y2>); +static_assert(!std::is_trivially_copy_assignable_v<std::optional<Y2>>); +static_assert(!std::is_trivially_move_assignable_v<std::optional<Y2>>); +static_assert(!std::is_trivially_copy_assignable_v<Z2>); +static_assert(std::is_trivially_move_assignable_v<Z2>); +static_assert(!std::is_trivially_copy_assignable_v<std::optional<Z2>>); +static_assert(!std::is_trivially_move_assignable_v<std::optional<Z2>>); + + +struct S { + S(const S&&) = delete; + S& operator=(const S&) = default; +}; +static_assert(std::is_trivially_copy_assignable_v<S>); + +union U { + char dummy; + S s; +}; +static_assert(std::is_trivially_copy_assignable_v<U>); + +static_assert(!std::is_trivially_copy_assignable_v<std::optional<S>>); +static_assert(!std::is_copy_assignable_v<std::optional<S>>); + +struct S2 { + S2(S2&&) = delete; + S2& operator=(const S2&) = default; +}; +static_assert(std::is_trivially_move_assignable_v<S2>); + +union U2 { + char dummy; + S2 s; +}; +static_assert(std::is_trivially_move_assignable_v<U2>); + +static_assert(!std::is_trivially_move_assignable_v<std::optional<S2>>); +static_assert(!std::is_move_assignable_v<std::optional<S2>>); 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..84a8e10 --- /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) 2018 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +#include <optional> + +struct X +{ + ~X(); +}; + +struct Y +{ + Y(const Y&) = default; + Y(Y&&); +}; + +struct Z +{ + Z(const Z&); + Z(Z&&) = default; +}; + +static_assert(std::is_trivially_copy_constructible_v<std::optional<int>>); +static_assert(std::is_trivially_move_constructible_v<std::optional<int>>); +static_assert(!std::is_trivially_copy_constructible_v<std::optional<X>>); +static_assert(!std::is_trivially_move_constructible_v<std::optional<X>>); +static_assert(std::is_trivially_copy_constructible_v<std::optional<Y>>); +static_assert(!std::is_trivially_move_constructible_v<std::optional<Y>>); +static_assert(!std::is_trivially_copy_constructible_v<std::optional<Z>>); +static_assert(std::is_trivially_move_constructible_v<std::optional<Z>>); 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 db0cc64..c448996 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 { *-*-* } } 495 } - // { dg-error "no type" "" { target { *-*-* } } 505 } - // { dg-error "no type" "" { target { *-*-* } } 562 } + // { dg-error "no type" "" { target { *-*-* } } 647 } + // { dg-error "no type" "" { target { *-*-* } } 657 } + // { dg-error "no type" "" { target { *-*-* } } 714 } } }
On 14/01/18 01:09 +0200, Ville Voutilainen wrote: >On 8 January 2018 at 15:36, Jonathan Wakely <jwakely@redhat.com> 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<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. > >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.
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 <typename _Tp, - bool /*_TrivialCopyMove*/ = - is_trivially_copy_constructible<_Tp>::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<typename... _Args> constexpr _Optional_payload(in_place_t, _Args&&... __args) @@ -131,7 +128,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION {} constexpr _Optional_payload(__ctor_tag<void>) - : _M_empty() + : _M_empty(), _M_engaged(false) {} constexpr _Optional_payload(__ctor_tag<bool>, _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 <typename _Tp> - 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_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 { @@ -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() { - 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(); + } } + }; - 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: + 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() + { + 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() + { + if (static_cast<_Dp*>(this)->_M_payload._M_engaged) + static_cast<_Dp*>(this)->_M_destruct(); + } + }; /** - * @brief Class template that holds the necessary state for @ref optional - * and that has the responsibility for construction and the special members. + * @brief Class template that takes care of copy/move constructors + of optional * * 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. - * + * conditionally make copy/move constructors trivial. * @see optional, _Enable_special_members */ - template<typename _Tp> + 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>> { - 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>; - + friend class _Optional_base_impl<_Tp, _Optional_base<_Tp>>; public: // Constructors for disengaged optionals. - constexpr _Optional_base() noexcept - { } - - constexpr _Optional_base(nullopt_t) noexcept - { } + constexpr _Optional_base() = default; // Constructors for engaged optionals. template<typename... _Args, @@ -347,84 +368,219 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { } // Assignment operators. - _Optional_base& - operator=(const _Optional_base& __other) + _Optional_base& operator=(const _Optional_base&) = default; + _Optional_base& operator=(_Optional_base&&) = default; + + protected: + + constexpr bool _M_is_engaged() const noexcept + { return this->_M_payload._M_engaged; } + + // The _M_get operations have _M_engaged as a precondition. + constexpr _Tp& + _M_get() noexcept { - if (this->_M_payload._M_engaged && __other._M_payload._M_engaged) - this->_M_get() = __other._M_get(); - else - { - if (__other._M_payload._M_engaged) - this->_M_construct(__other._M_get()); - else - this->_M_reset(); - } + __glibcxx_assert(this->_M_is_engaged()); + return this->_M_payload._M_payload; + } - return *this; + constexpr const _Tp& + _M_get() const noexcept + { + __glibcxx_assert(this->_M_is_engaged()); + return this->_M_payload._M_payload; } - _Optional_base& - operator=(_Optional_base&& __other) - noexcept(__and_<is_nothrow_move_constructible<_Tp>, - is_nothrow_move_assignable<_Tp>>()) + private: + _Optional_payload<_Tp> _M_payload; + }; + + template<typename _Tp> + class _Optional_base<_Tp, false, true> + : protected _Optional_base_impl<_Tp, _Optional_base<_Tp>> + { + friend class _Optional_base_impl<_Tp, _Optional_base<_Tp>>; + public: + + // Constructors for disengaged optionals. + constexpr _Optional_base() = default; + + // 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(in_place, + std::forward<_Args>(__args)...) { } + + template<typename _Up, typename... _Args, + enable_if_t<is_constructible_v<_Tp, + initializer_list<_Up>&, + _Args&&...>, bool> = false> + constexpr explicit _Optional_base(in_place_t, + initializer_list<_Up> __il, + _Args&&... __args) + : _M_payload(in_place, + __il, std::forward<_Args>(__args)...) + { } + + // Copy and move constructors. + constexpr _Optional_base(const _Optional_base& __other) + : _M_payload(__other._M_payload._M_engaged, + __other._M_payload) + { } + + constexpr _Optional_base(_Optional_base&& __other) = default; + + // Assignment operators. + _Optional_base& operator=(const _Optional_base&) = default; + _Optional_base& operator=(_Optional_base&&) = default; + + protected: + + constexpr bool _M_is_engaged() const noexcept + { return this->_M_payload._M_engaged; } + + // The _M_get operations have _M_engaged as a precondition. + constexpr _Tp& + _M_get() noexcept { - if (this->_M_payload._M_engaged && __other._M_payload._M_engaged) - this->_M_get() = std::move(__other._M_get()); - else - { - if (__other._M_payload._M_engaged) - this->_M_construct(std::move(__other._M_get())); - else - this->_M_reset(); - } - return *this; + __glibcxx_assert(this->_M_is_engaged()); + return this->_M_payload._M_payload; + } + + constexpr const _Tp& + _M_get() const noexcept + { + __glibcxx_assert(this->_M_is_engaged()); + return this->_M_payload._M_payload; } - // The following functionality is also needed by optional, hence the - // protected accessibility. + + private: + _Optional_payload<_Tp> _M_payload; + }; + + template<typename _Tp> + class _Optional_base<_Tp, true, false> + : protected _Optional_base_impl<_Tp, _Optional_base<_Tp>> + { + friend class _Optional_base_impl<_Tp, _Optional_base<_Tp>>; + public: + + // Constructors for disengaged optionals. + constexpr _Optional_base() = default; + + // 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(in_place, + std::forward<_Args>(__args)...) { } + + template<typename _Up, typename... _Args, + enable_if_t<is_constructible_v<_Tp, + initializer_list<_Up>&, + _Args&&...>, bool> = false> + constexpr explicit _Optional_base(in_place_t, + initializer_list<_Up> __il, + _Args&&... __args) + : _M_payload(in_place, + __il, std::forward<_Args>(__args)...) + { } + + // Copy and move constructors. + constexpr _Optional_base(const _Optional_base& __other) + = default; + + constexpr _Optional_base(_Optional_base&& __other) + noexcept(is_nothrow_move_constructible<_Tp>()) + : _M_payload(__other._M_payload._M_engaged, + std::move(__other._M_payload)) + { } + + // Assignment operators. + _Optional_base& operator=(const _Optional_base&) = default; + _Optional_base& operator=(_Optional_base&&) = default; + protected: + constexpr bool _M_is_engaged() const noexcept { return this->_M_payload._M_engaged; } // The _M_get operations have _M_engaged as a precondition. constexpr _Tp& - _M_get() noexcept + _M_get() noexcept { - __glibcxx_assert(_M_is_engaged()); + __glibcxx_assert(this->_M_is_engaged()); return this->_M_payload._M_payload; } constexpr const _Tp& - _M_get() const noexcept + _M_get() const noexcept { - __glibcxx_assert(_M_is_engaged()); + __glibcxx_assert(this->_M_is_engaged()); return this->_M_payload._M_payload; } - // 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._M_payload)) - _Stored_type(std::forward<_Args>(__args)...); - this->_M_payload._M_engaged = true; - } + private: + _Optional_payload<_Tp> _M_payload; + }; - void - _M_destruct() + template<typename _Tp> + class _Optional_base<_Tp, true, true> + : protected _Optional_base_impl<_Tp, _Optional_base<_Tp>> + { + friend class _Optional_base_impl<_Tp, _Optional_base<_Tp>>; + public: + + // Constructors for disengaged optionals. + constexpr _Optional_base() = default; + + // 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(in_place, + std::forward<_Args>(__args)...) { } + + template<typename _Up, typename... _Args, + enable_if_t<is_constructible_v<_Tp, + initializer_list<_Up>&, + _Args&&...>, bool> = false> + constexpr explicit _Optional_base(in_place_t, + initializer_list<_Up> __il, + _Args&&... __args) + : _M_payload(in_place, + __il, std::forward<_Args>(__args)...) + { } + + // Copy and move constructors. + constexpr _Optional_base(const _Optional_base& __other) + = default; + + constexpr _Optional_base(_Optional_base&& __other) = default; + + // Assignment operators. + _Optional_base& operator=(const _Optional_base&) = default; + _Optional_base& operator=(_Optional_base&&) = default; + + protected: + + constexpr bool _M_is_engaged() const noexcept + { return this->_M_payload._M_engaged; } + + // The _M_get operations have _M_engaged as a precondition. + constexpr _Tp& + _M_get() noexcept { - this->_M_payload._M_engaged = false; - this->_M_payload._M_payload.~_Stored_type(); + __glibcxx_assert(this->_M_is_engaged()); + return this->_M_payload._M_payload; } - // _M_reset is a 'safe' operation with no precondition. - void - _M_reset() + constexpr const _Tp& + _M_get() const noexcept { - if (this->_M_payload._M_engaged) - this->_M_destruct(); + __glibcxx_assert(this->_M_is_engaged()); + return this->_M_payload._M_payload; } private: @@ -482,8 +638,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION constexpr optional() = default; - constexpr optional(nullopt_t) noexcept - : _Base(nullopt) { } + constexpr optional(nullopt_t) noexcept { } // Converting constructors for engaged optionals. template <typename _Up = _Tp, 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. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +#include <optional> + +struct X +{ + ~X(); +}; + +struct Y +{ + Y& operator=(const Y&) = default; + Y& operator=(Y&&); + Y(const Y&) = default; + Y(Y&&) = default; +}; + +struct Z +{ + Z& operator=(const Z&); + Z& operator=(Z&&) = default; + Z(const Z&) = default; +}; + +struct Y2 +{ + Y2& operator=(const Y2&) = default; + Y2& operator=(Y2&&); +}; + +struct Z2 +{ + Z2& operator=(const Z2&); + Z2& operator=(Z2&&) = default; +}; + +static_assert(std::is_trivially_copy_assignable_v<std::optional<int>>); +static_assert(std::is_trivially_move_assignable_v<std::optional<int>>); +static_assert(!std::is_trivially_copy_assignable_v<std::optional<X>>); +static_assert(!std::is_trivially_move_assignable_v<std::optional<X>>); +static_assert(std::is_trivially_copy_assignable_v<std::optional<Y>>); +static_assert(!std::is_trivially_move_assignable_v<std::optional<Y>>); +static_assert(!std::is_trivially_copy_assignable_v<std::optional<Z>>); +static_assert(std::is_trivially_move_assignable_v<std::optional<Z>>); +static_assert(std::is_trivially_copy_assignable_v<Y2>); +static_assert(!std::is_trivially_move_assignable_v<Y2>); +static_assert(!std::is_trivially_copy_assignable_v<std::optional<Y2>>); +static_assert(!std::is_trivially_move_assignable_v<std::optional<Y2>>); +static_assert(!std::is_trivially_copy_assignable_v<Z2>); +static_assert(std::is_trivially_move_assignable_v<Z2>); +static_assert(!std::is_trivially_copy_assignable_v<std::optional<Z2>>); +static_assert(!std::is_trivially_move_assignable_v<std::optional<Z2>>); + + +struct S { + S(const S&&) = delete; + S& operator=(const S&) = default; +}; +static_assert(std::is_trivially_copy_assignable_v<S>); + +union U { + char dummy; + S s; +}; +static_assert(std::is_trivially_copy_assignable_v<U>); + +static_assert(!std::is_trivially_copy_assignable_v<std::optional<S>>); +static_assert(!std::is_copy_assignable_v<std::optional<S>>); + +struct S2 { + S2(S2&&) = delete; + S2& operator=(const S2&) = default; +}; +static_assert(std::is_trivially_move_assignable_v<S2>); + +union U2 { + char dummy; + S2 s; +}; +static_assert(std::is_trivially_move_assignable_v<U2>); + +static_assert(!std::is_trivially_move_assignable_v<std::optional<S2>>); +static_assert(!std::is_move_assignable_v<std::optional<S2>>); 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. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +#include <optional> + +struct X +{ + ~X(); +}; + +struct Y +{ + Y(const Y&) = default; + Y(Y&&); +}; + +struct Z +{ + Z(const Z&); + Z(Z&&) = default; +}; + +static_assert(std::is_trivially_copy_constructible_v<std::optional<int>>); +static_assert(std::is_trivially_move_constructible_v<std::optional<int>>); +static_assert(!std::is_trivially_copy_constructible_v<std::optional<X>>); +static_assert(!std::is_trivially_move_constructible_v<std::optional<X>>); +static_assert(std::is_trivially_copy_constructible_v<std::optional<Y>>); +static_assert(!std::is_trivially_move_constructible_v<std::optional<Y>>); +static_assert(!std::is_trivially_copy_constructible_v<std::optional<Z>>); +static_assert(std::is_trivially_move_constructible_v<std::optional<Z>>); 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 98964ea..3d60f96 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 { *-*-* } } 495 } - // { dg-error "no type" "" { target { *-*-* } } 505 } - // { dg-error "no type" "" { target { *-*-* } } 562 } + // { dg-error "no type" "" { target { *-*-* } } 650 } + // { dg-error "no type" "" { target { *-*-* } } 660 } + // { dg-error "no type" "" { target { *-*-* } } 717 } } }
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. So here you go (ho ho ho): 2017-12-25 Ville Voutilainen <ville.voutilainen@gmail.com> 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.