[v3] Fix C++0x pair constructors

[v3] Fix C++0x pair constructors

[Doug Gregor]

[-- Attachment #1: Type: text/plain, Size: 1974 bytes --]

The attached patch fixes the infamous "null pointer" problem that
Sylvain Pion found with the C++0x pair constructors:

#include <utility>

int main()
{
   std::pair<char *, char *> p (0,0);
}

The problem here is that the variadic constructor for the C++0x pair
turned the 0's into "int" values, and char* can't be initialized by an
int. So, this valid C++98 code fails to compile in C++0x. This patch
fixes that issue.

Working around this problem requires some extra pair constructors,
some enable_if magic, and Peter Dimov's cool trick of using enable_if
in the default template argument of a constructor template. The basic
idea is simple: use enable_if to steer the compiler away from any
constructors that might try to initialize a pointer with an integral
type. Instead, provide similar constructors that don't template that
argument, so that the compiler will convert the null pointer constant
to the pointer's type outside of the pair constructor. The details are
a little messy, because we need to make sure there's always a correct
fallback when the enable_if fails.

I thought there was a bug in Bugzilla where Sylvain reported this
problem, but I wasn't able to find it. This is exactly the problem
reported in LWG issue 811, which was resolved by the concepts
proposal. This patch is, effectively, trying to mimic what concepts
would do anyway.

Tested i686-pc-linux-gnu; no regressions.

  - Doug

2008-10-17  Douglas Gregor  <doug.gregor@gmail.com>

	* include/bits/stl_pair.h (__may_be_null_pointer_init): New.
	(pair::pair): Eliminate the redundant pair(U1&&, U2&&) constructor.
	Add lvalue pair<U1, U2> constructor to handle non-const pair lvalues.
	Remove the old variadic constructor, and instead provide several
	variadic constructors that avoid failing when attempting to
	initialize a pointer from a null pointer constant.
	* testsuite/20_util/pair/moveable.cc (test3): Add new tests with
	initialization of pointers from the null pointer constant.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: pair-null-pointer-fix.patch --]
[-- Type: text/x-patch; name=pair-null-pointer-fix.patch, Size: 6110 bytes --]

Index: include/bits/stl_pair.h
===================================================================
--- include/bits/stl_pair.h	(revision 141109)
+++ include/bits/stl_pair.h	(working copy)
@@ -65,8 +65,35 @@
 #include <bits/move.h> // for std::move / std::forward, std::decay, and
                        // std::swap
 
+#ifdef __GXX_EXPERIMENTAL_CXX0X__
+#include <type_traits>
+#endif
+
 _GLIBCXX_BEGIN_NAMESPACE(std)
 
+#ifdef __GXX_EXPERIMENTAL_CXX0X__
+// A trait that determines whether the initialization of a T from
+// arguments of type Args could possibly be the initialization of a
+// pointer from a null pointer constant.
+template<typename, typename...>
+struct __may_be_null_pointer_init 
+  : public false_type { };
+
+template<typename _Tp, typename _Up>
+struct __may_be_null_pointer_init<_Tp*, _Up>  
+  : public integral_constant<bool,
+             (is_integral<typename remove_reference<_Up>::type>::value 
+              || is_enum<typename remove_reference<_Up>::type>::value)> 
+  { };
+
+template<typename _Class, typename _Tp, typename _Up>
+struct __may_be_null_pointer_init<_Tp _Class::*, _Up>  
+  : public integral_constant<bool,
+             (is_integral<typename remove_reference<_Up>::type>::value 
+              || is_enum<typename remove_reference<_Up>::type>::value)> 
+  { };
+#endif
+
   /// pair holds two objects of arbitrary type.
   template<class _T1, class _T2>
     struct pair
@@ -89,10 +116,12 @@ _GLIBCXX_BEGIN_NAMESPACE(std)
       : first(__a), second(__b) { }
 
 #ifdef __GXX_EXPERIMENTAL_CXX0X__
-      template<class _U1, class _U2>
-        pair(_U1&& __x, _U2&& __y)
-	: first(std::forward<_U1>(__x)),
-	  second(std::forward<_U2>(__y)) { }
+      // Omitted the following constructor, which appears in the C++0x
+      // working paper but is redundant with the variadic constructors
+      // below.
+      //
+      //   template<class _U1, class _U2>
+      //     pair(_U1&& __x, _U2&& __y);
 
       pair(pair&& __p)
       : first(std::move(__p.first)),
@@ -111,12 +140,56 @@ _GLIBCXX_BEGIN_NAMESPACE(std)
 	: first(std::move(__p.first)),
 	  second(std::move(__p.second)) { }
 
-      // http://gcc.gnu.org/ml/libstdc++/2007-08/msg00052.html
-      template<class _U1, class _Arg0, class... _Args>
-        pair(_U1&& __x, _Arg0&& __arg0, _Args&&... __args)
+      // This constructor is required so that lvalue pairs don't get
+      // pushed to the variadic constructor below.
+      template<class _U1, class _U2>
+        pair(pair<_U1, _U2>& __p)
+          : first(const_cast<const pair<_U1, _U2>&>(__p).first),
+  	    second(const_cast<const pair<_U1, _U2>&>(__p).second) { }
+
+      // _GLIBCXX_RESOLVE_LIB_DEFECTS
+      // 811.  pair of pointers no longer works with literal 0
+
+      // Variadic constructor. The enable_if makes sure that we won't
+      // try to initialize a pointer from an integral type, which
+      // needs to be handled by a different constructor that will
+      // convert a null pointer constant to that pointer type. See
+      // library issue 767.
+      template<class _U1, class... _Args,
+        class _Checker 
+          = typename enable_if<
+                       (!__may_be_null_pointer_init<_T1, _U1>::value
+                        && !__may_be_null_pointer_init<_T2, _Args...>::value), 
+                     void>::type>
+        pair(_U1&& __x, _Args&&... __args)
+	: first(std::forward<_U1>(__x)),
+	  second(std::forward<_Args>(__args)...) { }
+
+      // Variadic constructor. The enable_if makes sure that the
+      // second argument isn't going to try to initialize a pointer
+      // from an integral type. However, T1 may be a pointer whose
+      // argument was a null pointer constant.
+      template<class... _Args,
+        class _Checker 
+          = typename enable_if<
+                       !__may_be_null_pointer_init<_T2, _Args...>::value, 
+                     void>::type>
+        pair(const _T1& __x, _Args&&... __args)
+	: first(__x),
+	  second(std::forward<_Args>(__args)...) { }
+
+      // Constructor typically used when T2 is a pointer and the
+      // second argument was a null pointer constant. The enable_if
+      // makes sure that the first argument isn't going to try to
+      // initialize a pointer from an integral type.
+      template<class _U1,
+        class _Checker 
+          = typename enable_if<
+                     !__may_be_null_pointer_init<_T1, _U1>::value,
+                     void>::type>
+        pair(_U1&& __x, const _T2& __y)
 	: first(std::forward<_U1>(__x)),
-	  second(std::forward<_Arg0>(__arg0),
-		 std::forward<_Args>(__args)...) { }
+	  second(__y) { }
 
       pair&
       operator=(pair&& __p)
Index: testsuite/20_util/pair/moveable.cc
===================================================================
--- testsuite/20_util/pair/moveable.cc	(revision 141109)
+++ testsuite/20_util/pair/moveable.cc	(working copy)
@@ -64,9 +64,54 @@ test2()
          r.second.size() == 2 && p.second.size() == 0);
 }
 
+struct X { 
+  explicit X(int, int) { }
+
+private:
+  X(const X&) = delete;
+};
+
+struct move_only {
+  move_only() { }
+  move_only(move_only&&) { }
+
+private:
+  move_only(const move_only&) = delete;
+};
+
+void
+test3()
+{
+  int *ip = 0;
+  int X::*mp = 0;
+  std::pair<int*, int*> p1(0, 0);
+  std::pair<int*, int*> p2(ip, 0);
+  std::pair<int*, int*> p3(0, ip);
+  std::pair<int*, int*> p4(ip, ip);
+
+  std::pair<int X::*, int*> p5(0, 0);
+  std::pair<int X::*, int X::*> p6(mp, 0);
+  std::pair<int X::*, int X::*> p7(0, mp);
+  std::pair<int X::*, int X::*> p8(mp, mp);
+
+  std::pair<int*, X> p9(0, 1, 2);
+  std::pair<int X::*, X> p10(0, 1, 2);
+  std::pair<int*, X> p11(ip, 1, 2);
+  std::pair<int X::*, X> p12(mp, 1, 2);
+
+  std::pair<int*, move_only> p13(0);
+  std::pair<int X::*, move_only> p14(0);
+
+  std::pair<int*, move_only> p15(0, move_only());
+  std::pair<int X::*, move_only> p16(0, move_only());
+  std::pair<move_only, int*> p17(move_only(), 0);
+  std::pair<move_only, int X::*> p18(move_only(), 0);
+}
+
 int 
 main() 
 {
   test1();
   test2();
+  test3();
 }

Re: [v3] Fix C++0x pair constructors

[Sylvain Pion]

Doug Gregor a écrit :
> I thought there was a bug in Bugzilla where Sylvain reported this
> problem, but I wasn't able to find it. This is exactly the problem
> reported in LWG issue 811, which was resolved by the concepts
> proposal. This patch is, effectively, trying to mimic what concepts
> would do anyway.

I don't think I reported anything to gcc bugzilla specifically for this.
The closest PR to this issue is PR34480, I think.

-- 
Sylvain Pion
INRIA Sophia-Antipolis
Geometrica Project-Team
CGAL, http://cgal.org/

Re: [v3] Fix C++0x pair constructors

[Paolo Carlini]

Doug Gregor wrote:
> Tested i686-pc-linux-gnu; no regressions.
>   
Approved for mainline of course, thanks Doug. Now I can also confidently
implement the "emplace" additions to map / multimap / ... which rely on
a sane std::pair.

Thanks again,
Paolo.

Re: [v3] Fix C++0x pair constructors

[Paolo Carlini]

Sylvain Pion wrote:
> The closest PR to this issue is PR34480, I think.
Indeed. Now, I see no reason to keep it open.

Paolo.

Re: [v3] Fix C++0x pair constructors

[Paolo Carlini]

On Oct 18, 2008, at 12:45 AM, Paolo Carlini wrote:

> Doug Gregor wrote:
>> Tested i686-pc-linux-gnu; no regressions.
>>
> Approved for mainline of course, thanks Doug. Now I can also  
> confidently
> implement the "emplace" additions to map / multimap / ... which rely  
> on
> a sane std::pair.

I went ahead and committed the patch to support the above-mentioned  
work on the containers...

Paolo.