Re: [v3 PATCH] Implement LWG 2900, The copy and move constructors of optional are not constexpr.

[Jonathan Wakely <jwakely at redhat dot com>]

On 17/03/17 16:51 +0200, Ville Voutilainen wrote:
> diff --git a/libstdc++-v3/include/std/optional b/libstdc++-v3/include/std/optional
> index 3f540ec..e67ba89 100644
> --- a/libstdc++-v3/include/std/optional
> +++ b/libstdc++-v3/include/std/optional
> @@ -95,125 +95,127 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> -      constexpr _Optional_base(nullopt_t) noexcept
> -      : _Optional_base{} { }
> +      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 {};

Newline here please, so it doesn't look like this template<...> is on
the constructor.

> +      constexpr _Optional_payload(__ctor_tag<bool>,
> +				  const _Tp& __other)
> +	: _M_payload(__other),
> +	  _M_engaged(true)
> +      {}
> +
> +      constexpr _Optional_payload(__ctor_tag<void>)
> +	: _M_empty()
> +      {}
> +
> +      constexpr _Optional_payload(__ctor_tag<bool>, _Tp&& __other)
> +	: _M_payload(std::move(__other)),
> +	  _M_engaged(true)
> +      {}
> +
> +      constexpr _Optional_payload(bool __engaged,
> +				  const _Optional_payload&

Looks like we don't need a newline before the parameter name here:

> +				  __other)
> +	: _Optional_payload(__engaged ?
> +			    _Optional_payload(__ctor_tag<bool>{},
> +					      __other._M_payload) :
> +			    _Optional_payload(__ctor_tag<void>{}))
> +      {}
> +
> +      constexpr _Optional_payload(bool __engaged,
> +				  _Optional_payload&&

Nor here:

> +				  __other)
> +	: _Optional_payload(__engaged ?
> +			    _Optional_payload(__ctor_tag<bool>{},
> +					      std::move(__other._M_payload)) :
> +			    _Optional_payload(__ctor_tag<void>{}))
> +      {}

Hmm, I see we don't actually show a conditional expression in the
Coding Style docs, but I'd do that as:

	: _Optional_payload(__engaged
			    ? _Optional_payload(__ctor_tag<bool>{},
                                               std::move(__other._M_payload))
			    : _Optional_payload(__ctor_tag<void>{}))


> -      // 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(std::forward<_Args>(__args)...), _M_engaged(true) { }
> +      using _Stored_type = remove_const_t<_Tp>;
> +      struct _Empty_byte { };

I was going to ask whether std::byte would be better here, but I think
a valueless struct is better.

> +      union {
> +          _Empty_byte _M_empty;
> +          _Stored_type _M_payload;
> +      };
> +      bool _M_engaged = false;
> +    };


> +      template<typename... _Args>
> +        void
> +        _M_construct(_Args&&... __args)
> +        noexcept(is_nothrow_constructible<_Stored_type, _Args...>())
> +        {
> +          ::new (std::__addressof(this->_M_payload))

I think we need (void*) here to ensure we use the reserved placement
form of operator new and not some other overload:

struct X { };
void* operator new(decltype(sizeof(0)) n, X*);

(We don't get this right everywhere that uses placement new with
arbitrary types).


> +            _Stored_type(std::forward<_Args>(__args)...);
> +          this->_M_engaged = true;
> +        }
>     };
>
> +      // 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))
> +          ::new (std::__addressof(this->_M_payload._M_payload))

Here too.

>             _Stored_type(std::forward<_Args>(__args)...);
> -          this->_M_engaged = true;
> +          this->_M_payload._M_engaged = true;
>         }

OK for trunk with those adjustements, thanks.