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.
