Re: [Patch] Implementation of n3793 <experimental/optional>

[Luc Danton <lucdanton at free dot fr>]

On 2013-10-31 14:22, Jonathan Wakely wrote:

Thank you for your feedback, parts of which I have omitted (but not 
ignored).

> [ snip ]
> >   - the ABI-related matters of bad_optional_access and
> >     __throw_bad_optional_access will need to be settled somehow (introduce
> >     src/experimental/optional.cc and
> >     src/experimental/compatibility_functexcept.cc? I can't really say), and
> > until
> >     then I have left the associated function definitions inline in the header
> > --
> >     they are marked by the only two XXX items of the file
> That's OK for now, but let's remember to revisit it some time!
> We usually don't bother marking definitions "inline" if they are
> implicitly inline due to being defined in the class body.
>
> [ snip patch lines ]
>
> I think we should also remove the "inline" specifier on the first
> declaration here, because when we move the definition to a .cc file it
> won't be inline anymore.

I wanted to put the emphasis on those inline definitions that shouldn't be,
but the // XXX items do that just as well and are as easily greppable. I 
agree
that it's a potential trap for the future, so no regrets.

> [ snip ]
>
> Could the SFINAE constraints on optional<T>::operator= and
> optional<T>::emplace be put on the return type, instead of as a
> parameter pack of the function?  Making it a parameter pack allows
> users to misuse the function by providing explicit template argument
> lists e.g. opt.emplace<int, 1, 2, 3, 4>(1) will compile, disabling the
> SFINAE constraint.  Arguably this falls into the category of "you
> can't protect against sufficiently motivated idiots" but putting the
> constraint on the return type makes it a non-issue.

No problem here. As an aside, I normally use something like

    enum struct enabled {};
    template<typename... Conditions>
    using EnableIf = typename std::enable_if<and_<Conditions...>::value,
enabled>::type;

which requires the user to really go out of their way to provide phony 
values.

> Shouldn't optional::swap() be using _M_construct(std::move(...)) when
> only one of the objects is engaged?

Yes. This is explicitly mentioned in the requirements for swap(), which 
I must have overlooked.
A test involving an optional move-only value would have caught that.

> I've attached a patch with these changes (except my suggestion for the
> constraints on operator= and emplace), and some re-indenting.  These
> are all trivial and so I think this is ready to commit. I can go ahead
> and do that for you if you like?

Looks good to me. I have attached your suggestion (applied over your own 
patch),
all tests pass.

>From 43f7450c55499450baa605e154e445b443ccffe6 Mon Sep 17 00:00:00 2001
From: mickk <mickk@example.com>
Date: Thu, 31 Oct 2013 19:10:25 +0100
Subject: [PATCH] Tweaked SFINAE constraints as suggested.

---
 libstdc++-v3/include/experimental/optional | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/libstdc++-v3/include/experimental/optional b/libstdc++-v3/include/experimental/optional
index d49e10a..5915892 100644
--- a/libstdc++-v3/include/experimental/optional
+++ b/libstdc++-v3/include/experimental/optional
@@ -496,12 +496,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
         return *this;
       }
 
-      template<typename _Up,
-               typename enable_if<is_same<_Tp,
-                                          typename decay<_Up>::type
-                                          >::value,
-                                  int>::type...>
-        optional&
+      template<typename _Up>
+        typename enable_if<
+                 is_same<_Tp, typename decay<_Up>::type>::value,
+                 optional&
+               >::type
         operator=(_Up&& __u)
         {
           static_assert(is_constructible<_Tp, _Up>()
@@ -527,13 +526,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  this->_M_construct(std::forward<_Args>(__args)...);
 	}
 
-      template<typename _Up,
-               typename... _Args,
-               typename enable_if<is_constructible<_Tp,
-                                                   initializer_list<_Up>&,
-                                                   _Args&&...>::value,
-                                  int>::type...>
-	void
+      template<typename _Up, typename... _Args>
+        typename enable_if<
+                 is_constructible<_Tp,
+                                  initializer_list<_Up>&,
+                                  _Args&&...>::value
+               >::type
 	emplace(initializer_list<_Up> __il, _Args&&... __args)
 	{
 	  this->_M_reset();
-- 
1.8.1.2