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.

Reply via email to