[v3 PATCH] Make optional conditionally trivially_{copy,move}_{constructible,assignable}

2017-12-25 Thread Ville Voutilainen 
In the midst of the holiday season, the king and ruler of all elves, otherwise
known as The Elf, was told by little elves that users are complaining how
stlstl and libc++ make optional's copy and move operations conditionally
trivial, but libstdc++ doesn't. This made The Elf fairly angry, and he spoke
"this will not stand".

Tested on Linux-PPC64. The change is an ABI break due to changing
optional to a trivially copyable type. It's perhaps
better to get that ABI break in now rather than later.

So here you go (ho ho ho):

2017-12-25  Ville Voutilainen  

Make optional conditionally
trivially_{copy,move}_{constructible,assignable}
* include/std/optional (_Optional_payload): Fix the comment in
the class head and turn into a primary and one specialization.
(_Optional_payload::_M_engaged): Strike the NSDMI.
(_Optional_payload<_Tp, false>::operator=(const _Optional_payload&)):
New.
(_Optional_payload<_Tp, false>::operator=(_Optional_payload&&)):
Likewise.
(_Optional_payload<_Tp, false>::_M_get): Likewise.
(_Optional_payload<_Tp, false>::_M_reset): Likewise.
(_Optional_base_impl): Likewise.
(_Optional_base): Turn into a primary and three specializations.
(optional(nullopt)): Change the base init.
* testsuite/20_util/optional/assignment/8.cc: New.
* testsuite/20_util/optional/cons/trivial.cc: Likewise.
* testsuite/20_util/optional/cons/value_neg.cc: Adjust.
diff --git a/libstdc++-v3/include/std/optional 
b/libstdc++-v3/include/std/optional
index e017eed..9d1e625 100644
--- a/libstdc++-v3/include/std/optional
+++ b/libstdc++-v3/include/std/optional
@@ -100,15 +100,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   // Payload for constexpr optionals.
   template ::value
- && is_trivially_move_constructible<_Tp>::value,
-   bool /*_ShouldProvideDestructor*/ =
+   bool /*_HasTrivialDestructor*/ =
  is_trivially_destructible<_Tp>::value>
 struct _Optional_payload
 {
   constexpr _Optional_payload()
-   : _M_empty() {}
+   : _M_empty(), _M_engaged(false) {}
 
   template
   constexpr _Optional_payload(in_place_t, _Args&&... __args)
@@ -131,7 +128,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   {}
 
   constexpr _Optional_payload(__ctor_tag)
-   : _M_empty()
+   : _M_empty(), _M_engaged(false)
   {}
 
   constexpr _Optional_payload(__ctor_tag, _Tp&& __other)
@@ -161,12 +158,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   _Empty_byte _M_empty;
   _Stored_type _M_payload;
   };
-  bool _M_engaged = false;
+  bool _M_engaged;
 };
 
-  // Payload for non-constexpr optionals with non-trivial destructor.
+  // Payload for optionals with non-trivial destructor.
   template 
-struct _Optional_payload<_Tp, false, false>
+struct _Optional_payload<_Tp, false>
 {
   constexpr _Optional_payload()
: _M_empty() {}
@@ -203,6 +200,39 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
  this->_M_construct(std::move(__other._M_payload));
   }
 
+  _Optional_payload&
+  operator=(const _Optional_payload& __other)
+  {
+if (this->_M_engaged && __other._M_engaged)
+  this->_M_get() = __other._M_get();
+else
+ {
+   if (__other._M_engaged)
+ this->_M_construct(__other._M_get());
+   else
+ this->_M_reset();
+ }
+
+return *this;
+  }
+
+  _Optional_payload&
+  operator=(_Optional_payload&& __other)
+  noexcept(__and_,
+ is_nothrow_move_assignable<_Tp>>())
+  {
+   if (this->_M_engaged && __other._M_engaged)
+ this->_M_get() = std::move(__other._M_get());
+   else
+ {
+   if (__other._M_engaged)
+ this->_M_construct(std::move(__other._M_get()));
+   else
+ this->_M_reset();
+ }
+   return *this;
+  }
+
   using _Stored_type = remove_const_t<_Tp>;
   struct _Empty_byte { };
   union {
@@ -226,95 +256,86 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 _Stored_type(std::forward<_Args>(__args)...);
   this->_M_engaged = true;
 }
-};
-
-  // Payload for non-constexpr optionals with trivial destructor.
-  template 
-struct _Optional_payload<_Tp, false, true>
-{
-  constexpr _Optional_payload()
-   : _M_empty() {}
-
-  template 
-  constexpr _Optional_payload(in_place_t, _Args&&... __args)
-   : _M_payload(std::forward<_Args>(__args)...),
- _M_engaged(true) {}
-
-  template
-  constexpr _Optional_payload(std::initializer_list<_Up> __il,
- _Args&&... __args)
-   : _M_payload(__il, std::forward<_Args>(__args)...),
- _M_engaged(true) {}
-  constexpr
-  _Optional_payload(bool __engaged, const _Optional_payload& __other)
-   : _Optional_payload(__other)
-  {}
 
-  constexpr
-  _Optional_payload(bool __engage

Re: [v3 PATCH] Make optional conditionally trivially_{copy,move}_{constructible,assignable}

2018-01-13 Thread Ville Voutilainen 
On 8 January 2018 at 15:36, Jonathan Wakely  wrote:
> On 25/12/17 23:59 +0200, Ville Voutilainen wrote:
>>
>> In the midst of the holiday season, the king and ruler of all elves,
>> otherwise
>> known as The Elf, was told by little elves that users are complaining how
>> stlstl and libc++ make optional's copy and move operations conditionally
>> trivial, but libstdc++ doesn't. This made The Elf fairly angry, and he
>> spoke
>> "this will not stand".
>>
>> Tested on Linux-PPC64. The change is an ABI break due to changing
>> optional to a trivially copyable type. It's perhaps
>> better to get that ABI break in now rather than later.
>
>
> Agreed, but a few comments and questions below.

New patch attached. I made _M_reset and _M_destruct noexcept, and
added a comment about the protected
inheritance in the code. Please double-check the whitespace department.
diff --git a/libstdc++-v3/include/std/optional 
b/libstdc++-v3/include/std/optional
index 466a11c..01fc06b 100644
--- a/libstdc++-v3/include/std/optional
+++ b/libstdc++-v3/include/std/optional
@@ -100,15 +100,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   // Payload for constexpr optionals.
   template ::value
- && is_trivially_move_constructible<_Tp>::value,
-   bool /*_ShouldProvideDestructor*/ =
+   bool /*_HasTrivialDestructor*/ =
  is_trivially_destructible<_Tp>::value>
 struct _Optional_payload
 {
   constexpr _Optional_payload()
-   : _M_empty() {}
+   : _M_empty(), _M_engaged(false) {}
 
   template
   constexpr _Optional_payload(in_place_t, _Args&&... __args)
@@ -131,7 +128,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   {}
 
   constexpr _Optional_payload(__ctor_tag)
-   : _M_empty()
+   : _M_empty(), _M_engaged(false)
   {}
 
   constexpr _Optional_payload(__ctor_tag, _Tp&& __other)
@@ -161,12 +158,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   _Empty_byte _M_empty;
   _Stored_type _M_payload;
   };
-  bool _M_engaged = false;
+  bool _M_engaged;
 };
 
-  // Payload for non-constexpr optionals with non-trivial destructor.
+  // Payload for optionals with non-trivial destructor.
   template 
-struct _Optional_payload<_Tp, false, false>
+struct _Optional_payload<_Tp, false>
 {
   constexpr _Optional_payload()
: _M_empty() {}
@@ -203,6 +200,38 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
  this->_M_construct(std::move(__other._M_payload));
   }
 
+  _Optional_payload&
+  operator=(const _Optional_payload& __other)
+  {
+if (this->_M_engaged && __other._M_engaged)
+  this->_M_get() = __other._M_get();
+else
+ {
+   if (__other._M_engaged)
+ this->_M_construct(__other._M_get());
+   else
+ this->_M_reset();
+ }
+   return *this;
+  }
+
+  _Optional_payload&
+  operator=(_Optional_payload&& __other)
+  noexcept(__and_,
+ is_nothrow_move_assignable<_Tp>>())
+  {
+   if (this->_M_engaged && __other._M_engaged)
+ this->_M_get() = std::move(__other._M_get());
+   else
+ {
+   if (__other._M_engaged)
+ this->_M_construct(std::move(__other._M_get()));
+   else
+ this->_M_reset();
+ }
+   return *this;
+  }
+
   using _Stored_type = remove_const_t<_Tp>;
   struct _Empty_byte { };
   union {
@@ -226,95 +255,87 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 _Stored_type(std::forward<_Args>(__args)...);
   this->_M_engaged = true;
 }
-};
 
-  // Payload for non-constexpr optionals with trivial destructor.
-  template 
-struct _Optional_payload<_Tp, false, true>
-{
-  constexpr _Optional_payload()
-   : _M_empty() {}
-
-  template 
-  constexpr _Optional_payload(in_place_t, _Args&&... __args)
-   : _M_payload(std::forward<_Args>(__args)...),
- _M_engaged(true) {}
-
-  template
-  constexpr _Optional_payload(std::initializer_list<_Up> __il,
- _Args&&... __args)
-   : _M_payload(__il, std::forward<_Args>(__args)...),
- _M_engaged(true) {}
-  constexpr
-  _Optional_payload(bool __engaged, const _Optional_payload& __other)
-   : _Optional_payload(__other)
-  {}
-
-  constexpr
-  _Optional_payload(bool __engaged, _Optional_payload&& __other)
-   : _Optional_payload(std::move(__other))
-  {}
-
-  constexpr _Optional_payload(const _Optional_payload& __other)
+  // The _M_get operations have _M_engaged as a precondition.
+  constexpr _Tp&
+   _M_get() noexcept
   {
-   if (__other._M_engaged)
- this->_M_construct(__other._M_payload);
+   return this->_M_payload;
   }
 
-  constexpr _Optional_payload(_Optional_payload&& __other)
+  constexpr const _Tp&
+   _M_get() const noexcept
   {
-   if (__other._M_engaged)
-

Re: [v3 PATCH] Make optional conditionally trivially_{copy,move}_{constructible,assignable}

2018-01-15 Thread Jonathan Wakely 

On 14/01/18 01:09 +0200, Ville Voutilainen wrote:

On 8 January 2018 at 15:36, Jonathan Wakely  wrote:

On 25/12/17 23:59 +0200, Ville Voutilainen wrote:


In the midst of the holiday season, the king and ruler of all elves,
otherwise
known as The Elf, was told by little elves that users are complaining how
stlstl and libc++ make optional's copy and move operations conditionally
trivial, but libstdc++ doesn't. This made The Elf fairly angry, and he
spoke
"this will not stand".

Tested on Linux-PPC64. The change is an ABI break due to changing
optional to a trivially copyable type. It's perhaps
better to get that ABI break in now rather than later.



Agreed, but a few comments and questions below.


New patch attached. I made _M_reset and _M_destruct noexcept, and
added a comment about the protected
inheritance in the code. Please double-check the whitespace department.


Thanks, OK for trunk.

Re: [v3 PATCH] Make optional conditionally trivially_{copy,move}_{constructible,assignable}

2018-01-08 Thread Jonathan Wakely 

On 25/12/17 23:59 +0200, Ville Voutilainen wrote:

In the midst of the holiday season, the king and ruler of all elves, otherwise
known as The Elf, was told by little elves that users are complaining how
stlstl and libc++ make optional's copy and move operations conditionally
trivial, but libstdc++ doesn't. This made The Elf fairly angry, and he spoke
"this will not stand".

Tested on Linux-PPC64. The change is an ABI break due to changing
optional to a trivially copyable type. It's perhaps
better to get that ABI break in now rather than later.


Agreed, but a few comments and questions below.



@@ -203,6 +200,39 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
  this->_M_construct(std::move(__other._M_payload));
  }

+  _Optional_payload&
+  operator=(const _Optional_payload& __other)
+  {
+if (this->_M_engaged && __other._M_engaged)
+  this->_M_get() = __other._M_get();
+else
+ {
+   if (__other._M_engaged)
+ this->_M_construct(__other._M_get());
+   else
+ this->_M_reset();
+ }
+
+return *this;
+  }
+
+  _Optional_payload&
+  operator=(_Optional_payload&& __other)
+  noexcept(__and_,
+ is_nothrow_move_assignable<_Tp>>())
+  {
+   if (this->_M_engaged && __other._M_engaged)
+ this->_M_get() = std::move(__other._M_get());
+   else
+ {
+   if (__other._M_engaged)
+ this->_M_construct(std::move(__other._M_get()));
+   else
+ this->_M_reset();
+ }
+   return *this;
+  }


Please make the whitespace before the return statement consistent in
these two assignment operators (one has a blank line and uses spaces,
one uses a tab).


@@ -226,95 +256,86 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_Stored_type(std::forward<_Args>(__args)...);
  this->_M_engaged = true;
}
-};
-
-  // Payload for non-constexpr optionals with trivial destructor.
-  template 
-struct _Optional_payload<_Tp, false, true>
-{
-  constexpr _Optional_payload()
-   : _M_empty() {}
-
-  template 
-  constexpr _Optional_payload(in_place_t, _Args&&... __args)
-   : _M_payload(std::forward<_Args>(__args)...),
- _M_engaged(true) {}
-
-  template
-  constexpr _Optional_payload(std::initializer_list<_Up> __il,
- _Args&&... __args)
-   : _M_payload(__il, std::forward<_Args>(__args)...),
- _M_engaged(true) {}
-  constexpr
-  _Optional_payload(bool __engaged, const _Optional_payload& __other)
-   : _Optional_payload(__other)
-  {}

-  constexpr
-  _Optional_payload(bool __engaged, _Optional_payload&& __other)
-   : _Optional_payload(std::move(__other))
-  {}
+  // The _M_get operations have _M_engaged as a precondition.
+  constexpr _Tp&
+   _M_get() noexcept
+  {
+   return this->_M_payload;
+  }

-  constexpr _Optional_payload(const _Optional_payload& __other)
+  constexpr const _Tp&
+   _M_get() const noexcept
  {
-   if (__other._M_engaged)
- this->_M_construct(__other._M_payload);
+   return this->_M_payload;
  }

-  constexpr _Optional_payload(_Optional_payload&& __other)
+  // _M_reset is a 'safe' operation with no precondition.
+  void
+  _M_reset()


Should this be noexcept?


  {
-   if (__other._M_engaged)
- this->_M_construct(std::move(__other._M_payload));
+   if (this->_M_engaged)
+ {
+   this->_M_engaged = false;
+   this->_M_payload.~_Stored_type();
+ }
  }
+  };


This closing brace seems to be indented incorrectly.


-  using _Stored_type = remove_const_t<_Tp>;
-  struct _Empty_byte { };
-  union {
-  _Empty_byte _M_empty;
-  _Stored_type _M_payload;
-  };
-  bool _M_engaged = false;
+  template
+class _Optional_base_impl
+  {
+  protected:


And thos whole class body should be indented to line up with the
"class" keyword.


+using _Stored_type = remove_const_t<_Tp>;
+
+// The _M_construct operation has !_M_engaged as a precondition
+// while _M_destruct has _M_engaged as a precondition.
+template
+void
+_M_construct(_Args&&... __args)
+  noexcept(is_nothrow_constructible<_Stored_type, _Args...>())
+{
+  ::new
+   (std::__addressof(static_cast<_Dp*>(this)->_M_payload._M_payload))
+   _Stored_type(std::forward<_Args>(__args)...);
+  static_cast<_Dp*>(this)->_M_payload._M_engaged = true;
+}

-  template
-void
-_M_construct(_Args&&... __args)
-noexcept(is_nothrow_constructible<_Stored_type, _Args...>())
-{
-  ::new ((void *) std::__addressof(this->_M_payload))
-_Stored_type(std::forward<_Args>(__args)...);
-  this->_M_engaged = true;
-}
-};
+void
+_M_destruct()


noexcept?


+