* [PATCH] PR libstdc++/87855 fix optional for types with non-trivial copy/move
@ 2019-01-08 23:00 Jonathan Wakely
2019-01-09 10:40 ` Jonathan Wakely
0 siblings, 1 reply; 2+ messages in thread
From: Jonathan Wakely @ 2019-01-08 23:00 UTC (permalink / raw)
To: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2579 bytes --]
When the contained value is not trivially copy (or move) constructible
the union's copy (or move) constructor will be deleted, and so the
_Optional_payload delegating constructors are invalid. G++ fails to
diagnose this because it incorrectly performs copy elision in the
delegating constructors. Clang does diagnose it (llvm.org/PR40245).
The solution is to avoid performing any copy (or move) when the
contained value's copy (or move) constructor isn't trivial. Instead the
contained value can be constructed by calling _M_construct. This is OK,
because the relevant constructor doesn't need to be constexpr when the
contained value isn't trivially copy (or move) constructible.
Additionally, this patch removes a lot of code duplication in the
_Optional_payload partial specializations and the _Optional_base partial
specialization, by hoisting it into common base classes.
The Python pretty printer for std::optional needs to be adjusted to
support the new layout. Retain support for the old layout, and add a
test to verify that the support still works.
PR libstdc++/87855
* include/std/optional (_Optional_payload_base): New class template
for common code hoisted from _Optional_payload specializations. Use
a template for the union, to allow a partial specialization for
types with non-trivial destructors. Add constructors for in-place
initialization to the union.
(_Optional_payload(bool, const _Optional_payload&)): Use _M_construct
to perform non-trivial copy construction, instead of relying on
non-standard copy elision in a delegating constructor.
(_Optional_payload(bool, _Optional_payload&&)): Likewise for
non-trivial move construction.
(_Optional_payload): Derive from _Optional_payload_base and use it
for everything except the non-trivial assignment operators, which are
defined as needed.
(_Optional_payload<false, C, M>): Derive from the specialization
_Optional_payload<true, false, false> and add a destructor.
(_Optional_base_impl::_M_destruct, _Optional_base_impl::_M_reset):
Forward to corresponding members of _Optional_payload.
(_Optional_base_impl::_M_is_engaged, _Optional_base_impl::_M_get):
Hoist common members from _Optional_base.
(_Optional_base): Make all members and base class public.
(_Optional_base::_M_get, _Optional_base::_M_is_engaged): Move to
_Optional_base_impl.
* python/libstdcxx/v6/printers.py (StdExpOptionalPrinter): Add
support for new std::optional layout.
* testsuite/libstdc++-prettyprinters/compat.cc: New test.
Tested x86_64-linux. Ville already ack'd this, so I'm committing it to trunk.
[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 37560 bytes --]
commit 5b7409eb9081b9c3efced73af2d386926b9b3dfe
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Tue Jan 8 13:12:32 2019 +0000
PR libstdc++/87855 fix optional for types with non-trivial copy/move
When the contained value is not trivially copy (or move) constructible
the union's copy (or move) constructor will be deleted, and so the
_Optional_payload delegating constructors are invalid. G++ fails to
diagnose this because it incorrectly performs copy elision in the
delegating constructors. Clang does diagnose it (llvm.org/PR40245).
The solution is to avoid performing any copy (or move) when the
contained value's copy (or move) constructor isn't trivial. Instead the
contained value can be constructed by calling _M_construct. This is OK,
because the relevant constructor doesn't need to be constexpr when the
contained value isn't trivially copy (or move) constructible.
Additionally, this patch removes a lot of code duplication in the
_Optional_payload partial specializations and the _Optional_base partial
specialization, by hoisting it into common base classes.
The Python pretty printer for std::optional needs to be adjusted to
support the new layout. Retain support for the old layout, and add a
test to verify that the support still works.
PR libstdc++/87855
* include/std/optional (_Optional_payload_base): New class template
for common code hoisted from _Optional_payload specializations. Use
a template for the union, to allow a partial specialization for
types with non-trivial destructors. Add constructors for in-place
initialization to the union.
(_Optional_payload(bool, const _Optional_payload&)): Use _M_construct
to perform non-trivial copy construction, instead of relying on
non-standard copy elision in a delegating constructor.
(_Optional_payload(bool, _Optional_payload&&)): Likewise for
non-trivial move construction.
(_Optional_payload): Derive from _Optional_payload_base and use it
for everything except the non-trivial assignment operators, which are
defined as needed.
(_Optional_payload<false, C, M>): Derive from the specialization
_Optional_payload<true, false, false> and add a destructor.
(_Optional_base_impl::_M_destruct, _Optional_base_impl::_M_reset):
Forward to corresponding members of _Optional_payload.
(_Optional_base_impl::_M_is_engaged, _Optional_base_impl::_M_get):
Hoist common members from _Optional_base.
(_Optional_base): Make all members and base class public.
(_Optional_base::_M_get, _Optional_base::_M_is_engaged): Move to
_Optional_base_impl.
* python/libstdcxx/v6/printers.py (StdExpOptionalPrinter): Add
support for new std::optional layout.
* testsuite/libstdc++-prettyprinters/compat.cc: New test.
diff --git a/libstdc++-v3/include/std/optional b/libstdc++-v3/include/std/optional
index 327e86e0409..b06e30fbd67 100644
--- a/libstdc++-v3/include/std/optional
+++ b/libstdc++-v3/include/std/optional
@@ -98,8 +98,155 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
__throw_bad_optional_access()
{ _GLIBCXX_THROW_OR_ABORT(bad_optional_access()); }
+ // This class template manages construction/destruction of
+ // the contained value for a std::optional.
+ template <typename _Tp>
+ struct _Optional_payload_base
+ {
+ using _Stored_type = remove_const_t<_Tp>;
- // Payload for optionals with non-trivial destructor.
+ _Optional_payload_base() = default;
+ ~_Optional_payload_base() = default;
+
+ template<typename... _Args>
+ constexpr
+ _Optional_payload_base(in_place_t __tag, _Args&&... __args)
+ : _M_payload(__tag, std::forward<_Args>(__args)...),
+ _M_engaged(true)
+ { }
+
+ template<typename _Up, typename... _Args>
+ constexpr
+ _Optional_payload_base(std::initializer_list<_Up> __il,
+ _Args&&... __args)
+ : _M_payload(__il, std::forward<_Args>(__args)...),
+ _M_engaged(true)
+ { }
+
+ // Constructor used by _Optional_base copy constructor when the
+ // contained value is not trivially copy constructible.
+ constexpr
+ _Optional_payload_base(bool __engaged,
+ const _Optional_payload_base& __other)
+ {
+ if (__other._M_engaged)
+ this->_M_construct(__other._M_get());
+ }
+
+ // Constructor used by _Optional_base move constructor when the
+ // contained value is not trivially move constructible.
+ constexpr
+ _Optional_payload_base(bool __engaged,
+ _Optional_payload_base&& __other)
+ {
+ if (__other._M_engaged)
+ this->_M_construct(std::move(__other._M_get()));
+ }
+
+ // Copy constructor is only used to when the contained value is
+ // trivially copy constructible.
+ _Optional_payload_base(const _Optional_payload_base&) = default;
+
+ // Move constructor is only used to when the contained value is
+ // trivially copy constructible.
+ _Optional_payload_base(_Optional_payload_base&&) = default;
+
+ _Optional_payload_base&
+ operator=(const _Optional_payload_base&) = default;
+
+ _Optional_payload_base&
+ operator=(_Optional_payload_base&&) = default;
+
+ struct _Empty_byte { };
+
+ template<typename _Up, bool = is_trivially_destructible_v<_Up>>
+ union _Storage
+ {
+ constexpr _Storage() noexcept : _M_empty() { }
+
+ template<typename... _Args>
+ constexpr
+ _Storage(in_place_t, _Args&&... __args)
+ : _M_value(std::forward<_Args>(__args)...)
+ { }
+
+ template<typename _Vp, typename... _Args>
+ constexpr
+ _Storage(std::initializer_list<_Vp> __il, _Args&&... __args)
+ : _M_value(__il, std::forward<_Args>(__args)...)
+ { }
+
+ _Empty_byte _M_empty;
+ _Up _M_value;
+ };
+
+ template<typename _Up>
+ union _Storage<_Up, false>
+ {
+ constexpr _Storage() noexcept : _M_empty() { }
+
+ template<typename... _Args>
+ constexpr
+ _Storage(in_place_t, _Args&&... __args)
+ : _M_value(std::forward<_Args>(__args)...)
+ { }
+
+ template<typename _Vp, typename... _Args>
+ constexpr
+ _Storage(std::initializer_list<_Vp> __il, _Args&&... __args)
+ : _M_value(__il, std::forward<_Args>(__args)...)
+ { }
+
+ // User-provided destructor is needed when _Up has non-trivial dtor.
+ ~_Storage() { }
+
+ _Empty_byte _M_empty;
+ _Up _M_value;
+ };
+
+ _Storage<_Stored_type> _M_payload;
+
+ bool _M_engaged = false;
+
+ template<typename... _Args>
+ void
+ _M_construct(_Args&&... __args)
+ noexcept(is_nothrow_constructible_v<_Stored_type, _Args...>)
+ {
+ ::new ((void *) std::__addressof(this->_M_payload))
+ _Stored_type(std::forward<_Args>(__args)...);
+ this->_M_engaged = true;
+ }
+
+ constexpr void
+ _M_destroy() noexcept
+ {
+ _M_engaged = false;
+ _M_payload._M_value.~_Stored_type();
+ }
+
+ // The _M_get() operations have _M_engaged as a precondition.
+ // They exist to access the contained value with the appropriate
+ // const-qualification, because _M_payload has had the const removed.
+
+ constexpr _Tp&
+ _M_get() noexcept
+ { return this->_M_payload._M_value; }
+
+ constexpr const _Tp&
+ _M_get() const noexcept
+ { return this->_M_payload._M_value; }
+
+ // _M_reset is a 'safe' operation with no precondition.
+ constexpr void
+ _M_reset() noexcept
+ {
+ if (this->_M_engaged)
+ _M_destroy();
+ }
+ };
+
+ // Class template that manages the payload for optionals.
template <typename _Tp,
bool /*_HasTrivialDestructor*/ =
is_trivially_destructible_v<_Tp>,
@@ -109,248 +256,32 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
bool /*_HasTrivialMove */ =
is_trivially_move_assignable_v<_Tp>
&& is_trivially_move_constructible_v<_Tp>>
- struct _Optional_payload
- {
- constexpr _Optional_payload() noexcept : _M_empty() { }
+ struct _Optional_payload;
- 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)
- {
- if (__other._M_engaged)
- this->_M_construct(__other._M_payload);
- }
-
- constexpr
- _Optional_payload(_Optional_payload&& __other)
- {
- if (__other._M_engaged)
- this->_M_construct(std::move(__other._M_payload));
- }
-
- constexpr
- _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;
- }
-
- constexpr
- _Optional_payload&
- operator=(_Optional_payload&& __other)
- noexcept(__and_v<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 {
- _Empty_byte _M_empty;
- _Stored_type _M_payload;
- };
- bool _M_engaged = false;
-
- ~_Optional_payload()
- {
- if (_M_engaged)
- _M_payload.~_Stored_type();
- }
-
- template<typename... _Args>
- void
- _M_construct(_Args&&... __args)
- noexcept(is_nothrow_constructible_v<_Stored_type, _Args...>)
- {
- ::new ((void *) std::__addressof(this->_M_payload))
- _Stored_type(std::forward<_Args>(__args)...);
- this->_M_engaged = true;
- }
-
- // The _M_get operations have _M_engaged as a precondition.
- constexpr _Tp&
- _M_get() noexcept
- { return this->_M_payload; }
-
- constexpr const _Tp&
- _M_get() const noexcept
- { return this->_M_payload; }
-
- // _M_reset is a 'safe' operation with no precondition.
- constexpr
- void
- _M_reset() noexcept
- {
- if (this->_M_engaged)
- {
- this->_M_engaged = false;
- this->_M_payload.~_Stored_type();
- }
- }
- };
-
- // Payload for potentially-constexpr optionals.
+ // Payload for potentially-constexpr optionals (trivial copy/move/destroy).
template <typename _Tp>
struct _Optional_payload<_Tp, true, true, true>
+ : _Optional_payload_base<_Tp>
{
- constexpr _Optional_payload() noexcept
- : _M_empty(), _M_engaged(false) { }
+ using _Optional_payload_base<_Tp>::_Optional_payload_base;
- 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)
- { }
-
- template <class _Up> struct __ctor_tag {};
-
- constexpr
- _Optional_payload(__ctor_tag<bool>, const _Tp& __other)
- : _M_payload(__other), _M_engaged(true)
- { }
-
- constexpr _Optional_payload(__ctor_tag<void>) noexcept
- : _M_empty(), _M_engaged(false)
- { }
-
- constexpr _Optional_payload(__ctor_tag<bool>, _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<bool>{},
- __other._M_payload) :
- _Optional_payload(__ctor_tag<void>{}))
- { }
-
- constexpr
- _Optional_payload(bool __engaged, _Optional_payload&& __other)
- : _Optional_payload(__engaged
- ? _Optional_payload(__ctor_tag<bool>{},
- std::move(__other._M_payload))
- : _Optional_payload(__ctor_tag<void>{}))
- { }
-
- using _Stored_type = remove_const_t<_Tp>;
-
- struct _Empty_byte { };
-
- union {
- _Empty_byte _M_empty;
- _Stored_type _M_payload;
- };
- bool _M_engaged;
+ _Optional_payload() = default;
};
- // Payload for optionals with non-trivial copy assignment.
+ // Payload for optionals with non-trivial copy construction/assignment.
template <typename _Tp>
struct _Optional_payload<_Tp, true, false, true>
+ : _Optional_payload_base<_Tp>
{
- constexpr _Optional_payload() noexcept
- : _M_empty(), _M_engaged(false) { }
-
- 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)
- { }
-
- template <class _Up> struct __ctor_tag {};
-
- constexpr _Optional_payload(__ctor_tag<bool>, const _Tp& __other)
- : _M_payload(__other),
- _M_engaged(true)
- { }
-
- constexpr _Optional_payload(__ctor_tag<void>) noexcept
- : _M_empty(), _M_engaged(false)
- { }
-
- constexpr _Optional_payload(__ctor_tag<bool>, _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<bool>{},
- __other._M_payload) :
- _Optional_payload(__ctor_tag<void>{}))
- { }
-
- constexpr
- _Optional_payload(bool __engaged, _Optional_payload&& __other)
- : _Optional_payload(__engaged
- ? _Optional_payload(__ctor_tag<bool>{},
- std::move(__other._M_payload))
- : _Optional_payload(__ctor_tag<void>{}))
- { }
+ using _Optional_payload_base<_Tp>::_Optional_payload_base;
+ _Optional_payload() = default;
+ ~_Optional_payload() = default;
_Optional_payload(const _Optional_payload&) = default;
_Optional_payload(_Optional_payload&&) = default;
+ _Optional_payload& operator=(_Optional_payload&&) = default;
+ // Non-trivial copy assignment.
constexpr
_Optional_payload&
operator=(const _Optional_payload& __other)
@@ -366,113 +297,22 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
}
return *this;
}
-
- _Optional_payload&
- operator=(_Optional_payload&& __other) = default;
-
- using _Stored_type = remove_const_t<_Tp>;
-
- struct _Empty_byte { };
-
- union {
- _Empty_byte _M_empty;
- _Stored_type _M_payload;
- };
- bool _M_engaged;
-
- template<typename... _Args>
- void
- _M_construct(_Args&&... __args)
- noexcept(is_nothrow_constructible_v<_Stored_type, _Args...>)
- {
- ::new ((void *) std::__addressof(this->_M_payload))
- _Stored_type(std::forward<_Args>(__args)...);
- this->_M_engaged = true;
- }
-
- // The _M_get operations have _M_engaged as a precondition.
- constexpr _Tp&
- _M_get() noexcept
- { return this->_M_payload; }
-
- constexpr const _Tp&
- _M_get() const noexcept
- { return this->_M_payload; }
-
- // _M_reset is a 'safe' operation with no precondition.
- constexpr
- void
- _M_reset() noexcept
- {
- if (this->_M_engaged)
- {
- this->_M_engaged = false;
- this->_M_payload.~_Stored_type();
- }
- }
};
- // Payload for optionals with non-trivial move assignment.
+ // Payload for optionals with non-trivial move construction/assignment.
template <typename _Tp>
struct _Optional_payload<_Tp, true, true, false>
+ : _Optional_payload_base<_Tp>
{
- constexpr _Optional_payload() noexcept
- : _M_empty(), _M_engaged(false) { }
-
- 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)
- { }
-
- template <class _Up> struct __ctor_tag {};
-
- constexpr
- _Optional_payload(__ctor_tag<bool>, const _Tp& __other)
- : _M_payload(__other),
- _M_engaged(true)
- { }
-
- constexpr _Optional_payload(__ctor_tag<void>) noexcept
- : _M_empty(), _M_engaged(false)
- { }
-
- constexpr _Optional_payload(__ctor_tag<bool>, _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<bool>{},
- __other._M_payload) :
- _Optional_payload(__ctor_tag<void>{}))
- { }
-
- constexpr
- _Optional_payload(bool __engaged, _Optional_payload&& __other)
- : _Optional_payload(__engaged
- ? _Optional_payload(__ctor_tag<bool>{},
- std::move(__other._M_payload))
- : _Optional_payload(__ctor_tag<void>{}))
- { }
+ using _Optional_payload_base<_Tp>::_Optional_payload_base;
+ _Optional_payload() = default;
+ ~_Optional_payload() = default;
_Optional_payload(const _Optional_payload&) = default;
_Optional_payload(_Optional_payload&&) = default;
+ _Optional_payload& operator=(const _Optional_payload&) = default;
- _Optional_payload&
- operator=(const _Optional_payload& __other) = default;
-
+ // Non-trivial move assignment.
constexpr
_Optional_payload&
operator=(_Optional_payload&& __other)
@@ -490,106 +330,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
}
return *this;
}
-
- using _Stored_type = remove_const_t<_Tp>;
-
- struct _Empty_byte { };
-
- union {
- _Empty_byte _M_empty;
- _Stored_type _M_payload;
- };
- bool _M_engaged;
-
- template<typename... _Args>
- void
- _M_construct(_Args&&... __args)
- noexcept(is_nothrow_constructible_v<_Stored_type, _Args...>)
- {
- ::new ((void *) std::__addressof(this->_M_payload))
- _Stored_type(std::forward<_Args>(__args)...);
- this->_M_engaged = true;
- }
-
- // The _M_get operations have _M_engaged as a precondition.
- constexpr _Tp&
- _M_get() noexcept
- { return this->_M_payload; }
-
- constexpr const _Tp&
- _M_get() const noexcept
- { return this->_M_payload; }
-
- // _M_reset is a 'safe' operation with no precondition.
- constexpr
- void
- _M_reset() noexcept
- {
- if (this->_M_engaged)
- {
- this->_M_engaged = false;
- this->_M_payload.~_Stored_type();
- }
- }
};
// Payload for optionals with non-trivial copy and move assignment.
template <typename _Tp>
struct _Optional_payload<_Tp, true, false, false>
+ : _Optional_payload_base<_Tp>
{
- constexpr _Optional_payload() noexcept
- : _M_empty(), _M_engaged(false) {}
-
- 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)
- { }
-
- template <class _Up> struct __ctor_tag {};
-
- constexpr _Optional_payload(__ctor_tag<bool>, const _Tp& __other)
- : _M_payload(__other),
- _M_engaged(true)
- { }
-
- constexpr _Optional_payload(__ctor_tag<void>) noexcept
- : _M_empty(), _M_engaged(false)
- { }
-
- constexpr _Optional_payload(__ctor_tag<bool>, _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<bool>{},
- __other._M_payload) :
- _Optional_payload(__ctor_tag<void>{}))
- { }
-
- constexpr
- _Optional_payload(bool __engaged, _Optional_payload&& __other)
- : _Optional_payload(__engaged
- ? _Optional_payload(__ctor_tag<bool>{},
- std::move(__other._M_payload))
- : _Optional_payload(__ctor_tag<void>{}))
- { }
+ using _Optional_payload_base<_Tp>::_Optional_payload_base;
+ _Optional_payload() = default;
+ ~_Optional_payload() = default;
_Optional_payload(const _Optional_payload&) = default;
_Optional_payload(_Optional_payload&&) = default;
+ // Non-trivial copy
constexpr
_Optional_payload&
operator=(const _Optional_payload& __other)
@@ -606,6 +361,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
return *this;
}
+ // Non-trivial move assignment.
constexpr
_Optional_payload&
operator=(_Optional_payload&& __other)
@@ -623,56 +379,33 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
}
return *this;
}
-
- using _Stored_type = remove_const_t<_Tp>;
-
- struct _Empty_byte { };
-
- union {
- _Empty_byte _M_empty;
- _Stored_type _M_payload;
- };
- bool _M_engaged;
-
- template<typename... _Args>
- constexpr
- void
- _M_construct(_Args&&... __args)
- noexcept(is_nothrow_constructible_v<_Stored_type, _Args...>)
- {
- ::new ((void *) std::__addressof(this->_M_payload))
- _Stored_type(std::forward<_Args>(__args)...);
- this->_M_engaged = true;
- }
-
- // The _M_get operations have _M_engaged as a precondition.
- constexpr _Tp&
- _M_get() noexcept
- { return this->_M_payload; }
-
- constexpr const _Tp&
- _M_get() const noexcept
- { return this->_M_payload; }
-
- // _M_reset is a 'safe' operation with no precondition.
- constexpr
- void
- _M_reset() noexcept
- {
- if (this->_M_engaged)
- {
- this->_M_engaged = false;
- this->_M_payload.~_Stored_type();
- }
- }
};
+ // Payload for optionals with non-trivial destructors.
+ template <typename _Tp, bool _Copy, bool _Move>
+ struct _Optional_payload<_Tp, false, _Copy, _Move>
+ : _Optional_payload<_Tp, true, false, false>
+ {
+ // Base class implements all the constructors and assignment operators:
+ using _Optional_payload<_Tp, true, false, false>::_Optional_payload;
+ _Optional_payload() = default;
+ _Optional_payload(const _Optional_payload&) = default;
+ _Optional_payload(_Optional_payload&&) = default;
+ _Optional_payload& operator=(const _Optional_payload&) = default;
+ _Optional_payload& operator=(_Optional_payload&&) = default;
+
+ // Destructor needs to destroy the contained value:
+ ~_Optional_payload() { this->_M_reset(); }
+ };
+
+ // Common base class for _Optional_base<T> to avoid repeating these
+ // member functions in each specialization.
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>
@@ -685,42 +418,59 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_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();
- }
-
+ { static_cast<_Dp*>(this)->_M_payload._M_destroy(); }
+
// _M_reset is a 'safe' operation with no precondition.
- constexpr
- void
+ constexpr void
_M_reset() noexcept
+ { static_cast<_Dp*>(this)->_M_payload._M_reset(); }
+
+ constexpr bool _M_is_engaged() const noexcept
+ { return static_cast<const _Dp*>(this)->_M_payload._M_engaged; }
+
+ // The _M_get operations have _M_engaged as a precondition.
+ constexpr _Tp&
+ _M_get() noexcept
{
- if (static_cast<_Dp*>(this)->_M_payload._M_engaged)
- static_cast<_Dp*>(this)->_M_destruct();
+ __glibcxx_assert(this->_M_is_engaged());
+ return static_cast<_Dp*>(this)->_M_payload._M_get();
}
- };
+
+ constexpr const _Tp&
+ _M_get() const noexcept
+ {
+ __glibcxx_assert(this->_M_is_engaged());
+ return static_cast<const _Dp*>(this)->_M_payload._M_get();
+ }
+ };
/**
- * @brief Class template that takes care of copy/move constructors
- of optional
+ * @brief Class template that provides copy/move constructors of optional.
*
* Such a separate base class template is necessary in order to
* conditionally make copy/move constructors trivial.
+ *
+ * When the contained value is trivally copy/move constructible,
+ * the copy/move constructors of _Optional_base will invoke the
+ * trivial copy/move constructor of _Optional_payload. Otherwise,
+ * they will invoke _Optional_payload(bool, const _Optional_payload&)
+ * or _Optional_payload(bool, _Optional_payload&&) to initialize
+ * the contained value, if copying/moving an engaged optional.
+ *
+ * Whether the other special members are trivial is determined by the
+ * _Optional_payload<_Tp> specialization used for the _M_payload member.
+ *
* @see optional, _Enable_special_members
*/
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>>
+ struct _Optional_base
+ : _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;
@@ -758,37 +508,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_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
- {
- __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;
- }
-
- private:
_Optional_payload<_Tp> _M_payload;
};
template<typename _Tp>
- class _Optional_base<_Tp, false, true>
- : protected _Optional_base_impl<_Tp, _Optional_base<_Tp>>
+ struct _Optional_base<_Tp, false, true>
+ : _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;
@@ -822,37 +548,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_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
- {
- __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;
- }
-
- private:
_Optional_payload<_Tp> _M_payload;
};
template<typename _Tp>
- class _Optional_base<_Tp, true, false>
- : protected _Optional_base_impl<_Tp, _Optional_base<_Tp>>
+ struct _Optional_base<_Tp, true, false>
+ : _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;
@@ -887,37 +589,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_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
- {
- __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;
- }
-
- private:
_Optional_payload<_Tp> _M_payload;
};
template<typename _Tp>
- class _Optional_base<_Tp, true, true>
- : protected _Optional_base_impl<_Tp, _Optional_base<_Tp>>
+ struct _Optional_base<_Tp, true, true>
+ : _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;
@@ -947,27 +625,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_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
- {
- __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;
- }
-
- private:
_Optional_payload<_Tp> _M_payload;
};
@@ -1262,36 +919,32 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
value() const&
{
return this->_M_is_engaged()
- ? this->_M_get()
- : (__throw_bad_optional_access(),
- this->_M_get());
+ ? this->_M_get()
+ : (__throw_bad_optional_access(), this->_M_get());
}
constexpr _Tp&
value()&
{
return this->_M_is_engaged()
- ? this->_M_get()
- : (__throw_bad_optional_access(),
- this->_M_get());
+ ? this->_M_get()
+ : (__throw_bad_optional_access(), this->_M_get());
}
constexpr _Tp&&
value()&&
{
return this->_M_is_engaged()
- ? std::move(this->_M_get())
- : (__throw_bad_optional_access(),
- std::move(this->_M_get()));
+ ? std::move(this->_M_get())
+ : (__throw_bad_optional_access(), std::move(this->_M_get()));
}
constexpr const _Tp&&
value() const&&
{
return this->_M_is_engaged()
- ? std::move(this->_M_get())
- : (__throw_bad_optional_access(),
- std::move(this->_M_get()));
+ ? std::move(this->_M_get())
+ : (__throw_bad_optional_access(), std::move(this->_M_get()));
}
template<typename _Up>
@@ -1302,8 +955,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
static_assert(is_convertible_v<_Up&&, _Tp>);
return this->_M_is_engaged()
- ? this->_M_get()
- : static_cast<_Tp>(std::forward<_Up>(__u));
+ ? this->_M_get() : static_cast<_Tp>(std::forward<_Up>(__u));
}
template<typename _Up>
diff --git a/libstdc++-v3/python/libstdcxx/v6/printers.py b/libstdc++-v3/python/libstdcxx/v6/printers.py
index 9e80e990e28..967d03a43c7 100644
--- a/libstdc++-v3/python/libstdcxx/v6/printers.py
+++ b/libstdc++-v3/python/libstdcxx/v6/printers.py
@@ -1093,13 +1093,23 @@ class StdExpOptionalPrinter(SingleObjContainerPrinter):
def __init__ (self, typename, val):
valtype = self._recognize (val.type.template_argument(0))
- self.typename = strip_versioned_namespace(typename)
- self.typename = re.sub('^std::(experimental::|)(fundamentals_v\d::|)(.*)', r'std::\1\3<%s>' % valtype, self.typename, 1)
- if not self.typename.startswith('std::experimental'):
- val = val['_M_payload']
- self.val = val
- contained_value = val['_M_payload'] if self.val['_M_engaged'] else None
- visualizer = gdb.default_visualizer (val['_M_payload'])
+ typename = strip_versioned_namespace(typename)
+ self.typename = re.sub('^std::(experimental::|)(fundamentals_v\d::|)(.*)', r'std::\1\3<%s>' % valtype, typename, 1)
+ payload = val['_M_payload']
+ if self.typename.startswith('std::experimental'):
+ engaged = val['_M_engaged']
+ contained_value = payload
+ else:
+ engaged = payload['_M_engaged']
+ contained_value = payload['_M_payload']
+ try:
+ # Since GCC 9
+ contained_value = contained_value['_M_value']
+ except:
+ pass
+ visualizer = gdb.default_visualizer (contained_value)
+ if not engaged:
+ contained_value = None
super (StdExpOptionalPrinter, self).__init__ (contained_value, visualizer)
def to_string (self):
diff --git a/libstdc++-v3/testsuite/libstdc++-prettyprinters/compat.cc b/libstdc++-v3/testsuite/libstdc++-prettyprinters/compat.cc
new file mode 100644
index 00000000000..39271ddaf27
--- /dev/null
+++ b/libstdc++-v3/testsuite/libstdc++-prettyprinters/compat.cc
@@ -0,0 +1,76 @@
+// { dg-options "-g -O0" }
+// { dg-do run }
+// { dg-skip-if "" { *-*-* } { "-D_GLIBCXX_PROFILE" } }
+
+// Copyright (C) 2014-2019 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/>.
+
+// Test that current printers still support old definitions of types.
+
+namespace std
+{
+ // Old representation of std::optional, before GCC 9
+ template<typename T>
+ struct _Optional_payload
+ {
+ _Optional_payload() : _M_empty(), _M_engaged(false) { }
+ struct _Empty_byte { };
+ union {
+ _Empty_byte _M_empty;
+ T _M_payload;
+ };
+ bool _M_engaged;
+ };
+
+ template<typename T>
+ struct _Optional_base
+ {
+ _Optional_payload<T> _M_payload;
+ };
+
+ template<typename T>
+ struct optional : _Optional_base<T>
+ {
+ optional() { }
+
+ optional(T t)
+ {
+ this->_M_payload._M_payload = t;
+ this->_M_payload._M_engaged = true;
+ }
+ };
+} // namespace std
+
+int
+main()
+{
+ using std::optional;
+
+ optional<int> o;
+// { dg-final { note-test o {std::optional<int> [no contained value]} } }
+ optional<bool> ob{false};
+// { dg-final { note-test ob {std::optional<bool> = {[contained value] = false}} } }
+ optional<int> oi{5};
+// { dg-final { note-test oi {std::optional<int> = {[contained value] = 5}} } }
+ optional<void*> op{nullptr};
+// { dg-final { note-test op {std::optional<void *> = {[contained value] = 0x0}} } }
+
+ __builtin_puts("");
+ return 0; // Mark SPOT
+}
+
+// { dg-final { gdb-test SPOT } }
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] PR libstdc++/87855 fix optional for types with non-trivial copy/move
2019-01-08 23:00 [PATCH] PR libstdc++/87855 fix optional for types with non-trivial copy/move Jonathan Wakely
@ 2019-01-09 10:40 ` Jonathan Wakely
0 siblings, 0 replies; 2+ messages in thread
From: Jonathan Wakely @ 2019-01-09 10:40 UTC (permalink / raw)
To: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]
On 08/01/19 23:00 +0000, Jonathan Wakely wrote:
>When the contained value is not trivially copy (or move) constructible
>the union's copy (or move) constructor will be deleted, and so the
>_Optional_payload delegating constructors are invalid. G++ fails to
>diagnose this because it incorrectly performs copy elision in the
>delegating constructors. Clang does diagnose it (llvm.org/PR40245).
>
>The solution is to avoid performing any copy (or move) when the
>contained value's copy (or move) constructor isn't trivial. Instead the
>contained value can be constructed by calling _M_construct. This is OK,
>because the relevant constructor doesn't need to be constexpr when the
>contained value isn't trivially copy (or move) constructible.
>
>Additionally, this patch removes a lot of code duplication in the
>_Optional_payload partial specializations and the _Optional_base partial
>specialization, by hoisting it into common base classes.
This follow-up patch removes a little more code duplication from those
partial specializations.
Tested x86_64-linux. Ville already ack'd this, so I'm committing it to trunk.
[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 4208 bytes --]
commit ffe7a5a74f406fe82b17eec9a68f8e079b5f43e1
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Wed Jan 9 10:37:06 2019 +0000
Remove some more code duplication in std::optional
Hoist the duplicated code from the _Optional_payload partial
specializations into the _Optional_payload_base base class.
* include/std/optional (_Optional_payload_base::_M_copy_assign): New
member function to perform non-trivial assignment.
(_Optional_payload_base::_M_move_assign): Likewise.
(_Optional_payload<T, true, false, true>::operator=)
(_Optional_payload<T, true, true, false>::operator=)
(_Optional_payload<T, true, false, false>::operator=): Call
_M_copy_assign and/or _M_move_assign to do non-trivial assignments.
diff --git a/libstdc++-v3/include/std/optional b/libstdc++-v3/include/std/optional
index b06e30fbd67..c5e66bdd140 100644
--- a/libstdc++-v3/include/std/optional
+++ b/libstdc++-v3/include/std/optional
@@ -157,6 +157,38 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_Optional_payload_base&
operator=(_Optional_payload_base&&) = default;
+ // used to perform non-trivial copy assignment.
+ constexpr void
+ _M_copy_assign(const _Optional_payload_base& __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();
+ }
+ }
+
+ // used to perform non-trivial move assignment.
+ constexpr void
+ _M_move_assign(_Optional_payload_base&& __other)
+ noexcept(__and_v<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();
+ }
+ }
+
struct _Empty_byte { };
template<typename _Up, bool = is_trivially_destructible_v<_Up>>
@@ -286,15 +318,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_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();
- }
+ this->_M_copy_assign(__other);
return *this;
}
};
@@ -319,15 +343,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
noexcept(__and_v<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();
- }
+ this->_M_move_assign(std::move(__other));
return *this;
}
};
@@ -344,20 +360,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_Optional_payload(const _Optional_payload&) = default;
_Optional_payload(_Optional_payload&&) = default;
- // Non-trivial copy
+ // Non-trivial copy assignment.
constexpr
_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();
- }
+ this->_M_copy_assign(__other);
return *this;
}
@@ -368,15 +376,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
noexcept(__and_v<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();
- }
+ this->_M_move_assign(std::move(__other));
return *this;
}
};
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-01-09 10:40 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-08 23:00 [PATCH] PR libstdc++/87855 fix optional for types with non-trivial copy/move Jonathan Wakely
2019-01-09 10:40 ` Jonathan Wakely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).