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.
commit ffe7a5a74f406fe82b17eec9a68f8e079b5f43e1
Author: Jonathan Wakely <jwak...@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;
}
};
