diff mbox

Implementation of n3793 <experimental/optional>

Message ID CAH6eHdTWkES_xn+3hN9n2mDWj6-orC8whW_RwaU13Oz5kr_nTw@mail.gmail.com
State New
Headers show

Commit Message

Jonathan Wakely Nov. 5, 2013, 3:51 p.m. UTC
On 3 November 2013 11:30, Paolo Carlini wrote:
> On 11/03/2013 12:19 PM, Jonathan Wakely wrote:
>>
>> Yes, Paolo pointed out these are failing on 32-bit targets, I've got a
>> patch coming. Luc is there any reason not to just replace all those large
>> constants with 0x1234ABCD, which fits in a long on 32-bit targets?
>
> By the way, that's what I did/hacked in my local tree ;)

I needed some other changes, otherwise comparing optional<long>{} ==
0x1234ABCD doesn't compile, because 0x1234ABCD is an int and
optional<T> doesn't support comparisons with anything except T.

> Jon, I have got a bunch of other minor tweaks, from proper formatting of
> conditional operator and some curly braces, to using __and_ and __not_ when
> possible, and other thingies, like no full stops at the end of asserts and
> throws. Passes testing. You may want to integrate it with your other changes
> in testing... Or I can wait and apply it myself.

Here's the combined patch, tested x86_64-linux, 64-bit and 32-bit,
committed to trunk.

2013-11-05  Jonathan Wakely  <jwakely.gcc@gmail.com>
            Paolo Carlini  <paolo.carlini@oracle.com>

        * include/experimental/optional: Use __and_<> and __not_<> in
        conditions. Style fixes.
        (__constexpr_addressof, swap): Make inline.
        * testsuite/experimental/optional/cons/copy.cc: Adjust constants for
        32-bit targets.
        * testsuite/experimental/optional/cons/move.cc: Likewise.
        * testsuite/experimental/optional/cons/value.cc: Likewise.
        * testsuite/experimental/optional/constexpr/cons/value.cc: Likewise.

Comments

Paolo Carlini Nov. 5, 2013, 3:56 p.m. UTC | #1
Hi,

On 11/05/2013 04:51 PM, Jonathan Wakely wrote:
> On 3 November 2013 11:30, Paolo Carlini wrote:
>> On 11/03/2013 12:19 PM, Jonathan Wakely wrote:
>>> Yes, Paolo pointed out these are failing on 32-bit targets, I've got a
>>> patch coming. Luc is there any reason not to just replace all those large
>>> constants with 0x1234ABCD, which fits in a long on 32-bit targets?
>> By the way, that's what I did/hacked in my local tree ;)
> I needed some other changes, otherwise comparing optional<long>{} ==
> 0x1234ABCD doesn't compile, because 0x1234ABCD is an int and
> optional<T> doesn't support comparisons with anything except T.
Yeah, I also had L suffixes.
>> Jon, I have got a bunch of other minor tweaks, from proper formatting of
>> conditional operator and some curly braces, to using __and_ and __not_ when
>> possible, and other thingies, like no full stops at the end of asserts and
>> throws. Passes testing. You may want to integrate it with your other changes
>> in testing... Or I can wait and apply it myself.
> Here's the combined patch, tested x86_64-linux, 64-bit and 32-bit,
Thanks!

Paolo.
diff mbox

Patch

diff --git a/libstdc++-v3/include/experimental/optional b/libstdc++-v3/include/experimental/optional
index 5915892..5882ff5 100644
--- a/libstdc++-v3/include/experimental/optional
+++ b/libstdc++-v3/include/experimental/optional
@@ -129,7 +129,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<typename _Tp>
     struct _Has_addressof_impl<_Tp,
       decltype( std::declval<const _Tp&>().operator&(), void() )>
-    : std::true_type { };
+      : std::true_type { };
 
   /**
     * @brief Trait that detects the presence of an overloaded unary operator&.
@@ -157,7 +157,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     */
   template<typename _Tp, typename enable_if<_Has_addressof<_Tp>::value,
                                             int>::type...>
-    _Tp* __constexpr_addressof(_Tp& __t)
+    inline _Tp* __constexpr_addressof(_Tp& __t)
     { return std::__addressof(__t); }
 
   /**
@@ -235,32 +235,31 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
         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();
-        }
+	  {
+	    if (__other._M_engaged)
+	      this->_M_construct(__other._M_get());
+	    else
+	      this->_M_reset();
+	  }
 
         return *this;
       }
 
       _Optional_base&
       operator=(_Optional_base&& __other)
-      noexcept(is_nothrow_move_constructible<_Tp>()
-               && is_nothrow_move_assignable<_Tp>())
+      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;
+	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;
       }
 
       // [X.Y.4.2] Destructor.
@@ -373,35 +372,33 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _Optional_base&
       operator=(const _Optional_base& __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;
+	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_base&
       operator=(_Optional_base&& __other)
-      noexcept(is_nothrow_move_constructible<_Tp>()
-               && is_nothrow_move_assignable<_Tp>())
+      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;
+	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;
       }
 
       // Sole difference
@@ -445,9 +442,10 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
     private:
       struct _Empty_byte { };
-      union {
-          _Empty_byte _M_empty;
-          _Stored_type _M_payload;
+      union
+      {
+	_Empty_byte _M_empty;
+	_Stored_type _M_payload;
       };
       bool _M_engaged = false;
     };
@@ -462,22 +460,20 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
         // Copy constructor.
         is_copy_constructible<_Tp>::value,
         // Copy assignment.
-        is_copy_constructible<_Tp>::value
-        && is_copy_assignable<_Tp>::value,
+        __and_<is_copy_constructible<_Tp>, is_copy_assignable<_Tp>>::value,
         // Move constructor.
         is_move_constructible<_Tp>::value,
         // Move assignment.
-        is_move_constructible<_Tp>::value
-        && is_move_assignable<_Tp>::value,
+        __and_<is_move_constructible<_Tp>, is_move_assignable<_Tp>>::value,
         // Unique tag type.
         optional<_Tp>>
     {
-      static_assert(!is_same<typename remove_cv<_Tp>::type,
-                             nullopt_t>()
-                    && !is_same<typename remove_cv<_Tp>::type,
-                             in_place_t>()
-                    && !is_reference<_Tp>(),
-                    "Invalid instantiation of optional<T>.");
+      static_assert(__and_<__not_<is_same<typename remove_cv<_Tp>::type,
+					  nullopt_t>>,
+			   __not_<is_same<typename remove_cv<_Tp>::type,
+					  in_place_t>>,
+			   __not_<is_reference<_Tp>>>(),
+                    "Invalid instantiation of optional<T>");
 
     private:
       using _Base = _Optional_base<_Tp>;
@@ -503,9 +499,9 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
                >::type
         operator=(_Up&& __u)
         {
-          static_assert(is_constructible<_Tp, _Up>()
-                        && is_assignable<_Tp&, _Up>(),
-                        "Cannot assign to value type from argument.");
+          static_assert(__and_<is_constructible<_Tp, _Up>,
+			       is_assignable<_Tp&, _Up>>(),
+                        "Cannot assign to value type from argument");
 
           if (this->_M_is_engaged())
             this->_M_get() = std::forward<_Up>(__u);
@@ -520,7 +516,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	emplace(_Args&&... __args)
 	{
 	  static_assert(is_constructible<_Tp, _Args&&...>(),
-			"Cannot emplace value type from arguments.");
+			"Cannot emplace value type from arguments");
 
 	  this->_M_reset();
 	  this->_M_construct(std::forward<_Args>(__args)...);
@@ -551,15 +547,15 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
         if (this->_M_is_engaged() && __other._M_is_engaged())
           swap(this->_M_get(), __other._M_get());
         else if (this->_M_is_engaged())
-        {
-          __other._M_construct(std::move(this->_M_get()));
-          this->_M_destruct();
-        }
+	  {
+	    __other._M_construct(std::move(this->_M_get()));
+	    this->_M_destruct();
+	  }
         else if (__other._M_is_engaged())
-        {
-          this->_M_construct(std::move(__other._M_get()));
-          __other._M_destruct();
-        }
+	  {
+	    this->_M_construct(std::move(__other._M_get()));
+	    __other._M_destruct();
+	  }
       }
 
       // [X.Y.4.5] Observers.
@@ -585,11 +581,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       constexpr const _Tp&
       value() const
       {
-        return this->_M_is_engaged() ?
-          this->_M_get() :
-          (__throw_bad_optional_access("Attempt to access value of a disengaged"
-                                       " optional object."),
-           this->_M_get());
+	return this->_M_is_engaged()
+	  ?  this->_M_get()
+	  : (__throw_bad_optional_access("Attempt to access value of a "
+		                         "disengaged optional object"),
+	     this->_M_get());
       }
 
       _Tp&
@@ -598,34 +594,34 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
         if (this->_M_is_engaged())
           return this->_M_get();
 
-        __throw_bad_optional_access("Attempt to access value of a disengaged"
-                                    " optional object.");
+        __throw_bad_optional_access("Attempt to access value of a "
+				    "disengaged optional object");
       }
 
       template<typename _Up>
 	constexpr _Tp
 	value_or(_Up&& __u) const&
 	{
-	  static_assert(is_copy_constructible<_Tp>()
-			&& is_convertible<_Up&&, _Tp>(),
-			"Cannot return value.");
+	  static_assert(__and_<is_copy_constructible<_Tp>,
+			       is_convertible<_Up&&, _Tp>>(),
+			"Cannot return value");
 
-	  return this->_M_is_engaged() ?
-	    this->_M_get() :
-	    static_cast<_Tp>(std::forward<_Up>(__u));
+	  return this->_M_is_engaged()
+	    ? this->_M_get()
+	    : static_cast<_Tp>(std::forward<_Up>(__u));
 	}
 
       template<typename _Up>
 	_Tp
 	value_or(_Up&& __u) &&
 	{
-	  static_assert( is_move_constructible<_Tp>()
-			&& is_convertible<_Up&&, _Tp>(),
-			"Cannot return value." );
+	  static_assert(__and_<is_move_constructible<_Tp>,
+			       is_convertible<_Up&&, _Tp>>(),
+			"Cannot return value" );
 
-	  return this->_M_is_engaged() ?
-	    std::move(this->_M_get()) :
-	    static_cast<_Tp>(std::forward<_Up>(__u));
+	  return this->_M_is_engaged()
+	    ? std::move(this->_M_get())
+	    : static_cast<_Tp>(std::forward<_Up>(__u));
 	}
     };
 
@@ -635,7 +631,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     operator==(const optional<_Tp>& __lhs, const optional<_Tp>& __rhs)
     {
       return static_cast<bool>(__lhs) == static_cast<bool>(__rhs)
-        && (!__lhs || *__lhs == *__rhs);
+	     && (!__lhs || *__lhs == *__rhs);
     }
 
   template<typename _Tp>
@@ -647,8 +643,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     constexpr bool
     operator<(const optional<_Tp>& __lhs, const optional<_Tp>& __rhs)
     {
-      return static_cast<bool>(__rhs)
-        && (!__lhs || *__lhs < *__rhs);
+      return static_cast<bool>(__rhs) && (!__lhs || *__lhs < *__rhs);
     }
 
   template<typename _Tp>
@@ -790,7 +785,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   // [X.Y.11]
   template<typename _Tp>
-    void
+    inline void
     swap(optional<_Tp>& __lhs, optional<_Tp>& __rhs)
     noexcept(noexcept(__lhs.swap(__rhs)))
     { __lhs.swap(__rhs); }
diff --git a/libstdc++-v3/testsuite/experimental/optional/cons/copy.cc b/libstdc++-v3/testsuite/experimental/optional/cons/copy.cc
index bbd9328..53c23a1 100644
--- a/libstdc++-v3/testsuite/experimental/optional/cons/copy.cc
+++ b/libstdc++-v3/testsuite/experimental/optional/cons/copy.cc
@@ -63,11 +63,12 @@  int main()
   }
 
   {
-    std::experimental::optional<long> o { std::experimental::in_place, 0x1234ABCDF1E2D3C4 };
+    const long val = 0x1234ABCD;
+    std::experimental::optional<long> o { std::experimental::in_place, val};
     auto copy = o;
     VERIFY( copy );
-    VERIFY( *copy == 0x1234ABCDF1E2D3C4 );
-    VERIFY( o && o == 0x1234ABCDF1E2D3C4 );
+    VERIFY( *copy == val );
+    VERIFY( o && o == val );
   }
 
   {
diff --git a/libstdc++-v3/testsuite/experimental/optional/cons/move.cc b/libstdc++-v3/testsuite/experimental/optional/cons/move.cc
index 75d7677..ba31699 100644
--- a/libstdc++-v3/testsuite/experimental/optional/cons/move.cc
+++ b/libstdc++-v3/testsuite/experimental/optional/cons/move.cc
@@ -63,11 +63,12 @@  int main()
   }
 
   {
-    std::experimental::optional<long> o { std::experimental::in_place, 0x1234ABCDF1E2D3C4 };
+    const long val = 0x1234ABCD;
+    std::experimental::optional<long> o { std::experimental::in_place, val};
     auto moved_to = std::move(o);
     VERIFY( moved_to );
-    VERIFY( *moved_to == 0x1234ABCDF1E2D3C4 );
-    VERIFY( o && *o == 0x1234ABCDF1E2D3C4 );
+    VERIFY( *moved_to == val );
+    VERIFY( o && *o == val );
   }
 
   {
diff --git a/libstdc++-v3/testsuite/experimental/optional/cons/value.cc b/libstdc++-v3/testsuite/experimental/optional/cons/value.cc
index 339c120..1eba62d 100644
--- a/libstdc++-v3/testsuite/experimental/optional/cons/value.cc
+++ b/libstdc++-v3/testsuite/experimental/optional/cons/value.cc
@@ -66,51 +66,51 @@  int main()
   // [20.5.4.1] Constructors
 
   {
-    auto i = 0x1234ABCDF1E2D3C4;
+    auto i = 0x1234ABCD;
     std::experimental::optional<long> o { i };
     VERIFY( o );
-    VERIFY( *o == 0x1234ABCDF1E2D3C4 );
-    VERIFY( i == 0x1234ABCDF1E2D3C4 );
+    VERIFY( *o == 0x1234ABCD );
+    VERIFY( i == 0x1234ABCD );
   }
 
   {
-    auto i = 0x1234ABCDF1E2D3C4;
+    auto i = 0x1234ABCD;
     std::experimental::optional<long> o = i;
     VERIFY( o );
-    VERIFY( *o == 0x1234ABCDF1E2D3C4 );
-    VERIFY( i == 0x1234ABCDF1E2D3C4 );
+    VERIFY( *o == 0x1234ABCD );
+    VERIFY( i == 0x1234ABCD );
   }
 
   {
-    auto i = 0x1234ABCDF1E2D3C4;
+    auto i = 0x1234ABCD;
     std::experimental::optional<long> o = { i };
     VERIFY( o );
-    VERIFY( *o == 0x1234ABCDF1E2D3C4 );
-    VERIFY( i == 0x1234ABCDF1E2D3C4 );
+    VERIFY( *o == 0x1234ABCD );
+    VERIFY( i == 0x1234ABCD );
   }
 
   {
-    auto i = 0x1234ABCDF1E2D3C4;
+    auto i = 0x1234ABCD;
     std::experimental::optional<long> o { std::move(i) };
     VERIFY( o );
-    VERIFY( *o == 0x1234ABCDF1E2D3C4 );
-    VERIFY( i == 0x1234ABCDF1E2D3C4 );
+    VERIFY( *o == 0x1234ABCD );
+    VERIFY( i == 0x1234ABCD );
   }
 
   {
-    auto i = 0x1234ABCDF1E2D3C4;
+    auto i = 0x1234ABCD;
     std::experimental::optional<long> o = std::move(i);
     VERIFY( o );
-    VERIFY( *o == 0x1234ABCDF1E2D3C4 );
-    VERIFY( i == 0x1234ABCDF1E2D3C4 );
+    VERIFY( *o == 0x1234ABCD );
+    VERIFY( i == 0x1234ABCD );
   }
 
   {
-    auto i = 0x1234ABCDF1E2D3C4;
+    auto i = 0x1234ABCD;
     std::experimental::optional<long> o = { std::move(i) };
     VERIFY( o );
-    VERIFY( *o == 0x1234ABCDF1E2D3C4 );
-    VERIFY( i == 0x1234ABCDF1E2D3C4 );
+    VERIFY( *o == 0x1234ABCD );
+    VERIFY( i == 0x1234ABCD );
   }
 
   {
diff --git a/libstdc++-v3/testsuite/experimental/optional/constexpr/cons/value.cc b/libstdc++-v3/testsuite/experimental/optional/constexpr/cons/value.cc
index 98e0a22..eb7233b 100644
--- a/libstdc++-v3/testsuite/experimental/optional/constexpr/cons/value.cc
+++ b/libstdc++-v3/testsuite/experimental/optional/constexpr/cons/value.cc
@@ -26,44 +26,44 @@  int main()
   // [20.5.4.1] Constructors
 
   {
-    constexpr auto i = 0x1234ABCDF1E2D3C4;
+    constexpr long i = 0x1234ABCD;
     constexpr std::experimental::optional<long> o { i };
     static_assert( o, "" );
-    static_assert( *o == 0x1234ABCDF1E2D3C4, "" );
+    static_assert( *o == 0x1234ABCD, "" );
   }
 
   {
-    constexpr auto i = 0x1234ABCDF1E2D3C4;
+    constexpr long i = 0x1234ABCD;
     constexpr std::experimental::optional<long> o = i;
     static_assert( o, "" );
-    static_assert( *o == 0x1234ABCDF1E2D3C4, "" );
+    static_assert( *o == 0x1234ABCD, "" );
   }
 
   {
-    constexpr auto i = 0x1234ABCDF1E2D3C4;
+    constexpr long i = 0x1234ABCD;
     constexpr std::experimental::optional<long> o = { i };
     static_assert( o, "" );
-    static_assert( *o == 0x1234ABCDF1E2D3C4, "" );
+    static_assert( *o == 0x1234ABCD, "" );
   }
 
   {
-    constexpr auto i = 0x1234ABCDF1E2D3C4;
+    constexpr long i = 0x1234ABCD;
     constexpr std::experimental::optional<long> o { std::move(i) };
     static_assert( o, "" );
-    static_assert( *o == 0x1234ABCDF1E2D3C4, "" );
+    static_assert( *o == 0x1234ABCD, "" );
   }
 
   {
-    constexpr auto i = 0x1234ABCDF1E2D3C4;
+    constexpr long i = 0x1234ABCD;
     constexpr std::experimental::optional<long> o = std::move(i);
     static_assert( o, "" );
-    static_assert( *o == 0x1234ABCDF1E2D3C4, "" );
+    static_assert( *o == 0x1234ABCD, "" );
   }
 
   {
-    constexpr auto i = 0x1234ABCDF1E2D3C4;
+    constexpr long i = 0x1234ABCD;
     constexpr std::experimental::optional<long> o = { std::move(i) };
     static_assert( o, "" );
-    static_assert( *o == 0x1234ABCDF1E2D3C4, "" );
+    static_assert( *o == 0x1234ABCD, "" );
   }
 }