This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] PR libstdc++/86751 default assignment operators for std::pair


On 31/07/18 23:34 +0100, Jonathan Wakely wrote:
On 31/07/18 18:40 +0100, Jonathan Wakely wrote:
On 31/07/18 20:14 +0300, Ville Voutilainen wrote:
On 31 July 2018 at 20:07, Jonathan Wakely <jwakely@redhat.com> wrote:
The solution for PR 77537 causes ambiguities due to the extra copy
assignment operator taking a __nonesuch_no_braces parameter. The copy
and move assignment operators can be defined as defaulted to meet the
semantics required by the standard.

In order to preserve ABI compatibility (specifically argument passing
conventions for pair<T, T>) we need a new base class that makes the
assignment operators non-trivial.

      PR libstdc++/86751
      * include/bits/stl_pair.h (__nonesuch_no_braces): Remove.
      (__pair_base): New class with non-trivial copy assignment operator.
      (pair): Derive from __pair_base. Define copy assignment and move
      assignment operators as defaulted.
      * testsuite/20_util/pair/86751.cc: New test.


Ville, this passes all our tests, but am I forgetting something that
means this isn't right?

Pairs of references?

I knew there was a reason.

We need better tests, since nothing failed when I made this change.

OK, let me rework the patch ...

Here's the patch I've committed. It adds a test for pairs of
references, so I don't try to define t he assignment ops as defaulted
again :-)  Thanks for the quick feedback for these patches.

Tested powerpc64le-linux, committed to trunk.

This is a regression on all branches, but I'd like to leave it on
trunk for a short while before backporting it.

Now backported to all active branches.

commit 988a9158fd074353621f4f216270109c767a4725
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Jul 31 17:26:04 2018 +0100

   PR libstdc++/86751 default assignment operators for std::pair
The solution for PR 77537 causes ambiguities due to the extra copy
   assignment operator taking a __nonesuch_no_braces parameter. By making
   the base class non-assignable we don't need the extra deleted overload
   in std::pair. The copy assignment operator will be implicitly deleted
   (and the move assignment operator not declared) as needed. Without the
   additional user-provided operator in std::pair the ambiguity is avoided.
PR libstdc++/86751
           * include/bits/stl_pair.h (__pair_base): New class with deleted copy
           assignment operator.
           (pair): Derive from __pair_base.
           (pair::operator=): Remove deleted overload.
           * python/libstdcxx/v6/printers.py (StdPairPrinter): New pretty printer
           so that new base class isn't shown in GDB.
           * testsuite/20_util/pair/86751.cc: New test.
           * testsuite/20_util/pair/ref_assign.cc: New test.

diff --git a/libstdc++-v3/include/bits/stl_pair.h b/libstdc++-v3/include/bits/stl_pair.h
index a2486ba8244..ea8bd981559 100644
--- a/libstdc++-v3/include/bits/stl_pair.h
+++ b/libstdc++-v3/include/bits/stl_pair.h
@@ -185,8 +185,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
  struct __nonesuch_no_braces : std::__nonesuch {
    explicit __nonesuch_no_braces(const __nonesuch&) = delete;
  };
+#endif // C++11

-#endif
+  class __pair_base
+  {
+#if __cplusplus >= 201103L
+    template<typename _T1, typename _T2> friend struct pair;
+    __pair_base() = default;
+    ~__pair_base() = default;
+    __pair_base(const __pair_base&) = default;
+    __pair_base& operator=(const __pair_base&) = delete;
+#endif // C++11
+  };

 /**
   *  @brief Struct holding two objects of arbitrary type.
@@ -196,6 +206,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   */
  template<typename _T1, typename _T2>
    struct pair
+    : private __pair_base
    {
      typedef _T1 first_type;    /// @c first_type is the first bound type
      typedef _T2 second_type;   /// @c second_type is the second bound type
@@ -374,19 +385,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
	return *this;
      }

-      pair&
-      operator=(typename conditional<
-		__not_<__and_<is_copy_assignable<_T1>,
-		              is_copy_assignable<_T2>>>::value,
-		const pair&, const __nonesuch_no_braces&>::type __p) = delete;
-
      pair&
      operator=(typename conditional<
		__and_<is_move_assignable<_T1>,
		       is_move_assignable<_T2>>::value,
		pair&&, __nonesuch_no_braces&&>::type __p)
      noexcept(__and_<is_nothrow_move_assignable<_T1>,
-	              is_nothrow_move_assignable<_T2>>::value)
+		      is_nothrow_move_assignable<_T2>>::value)
      {
	first = std::forward<first_type>(__p.first);
	second = std::forward<second_type>(__p.second);
diff --git a/libstdc++-v3/python/libstdcxx/v6/printers.py b/libstdc++-v3/python/libstdcxx/v6/printers.py
index 34d8b4e6606..43d459ec8ec 100644
--- a/libstdc++-v3/python/libstdcxx/v6/printers.py
+++ b/libstdc++-v3/python/libstdcxx/v6/printers.py
@@ -1229,6 +1229,39 @@ class StdExpPathPrinter:
        return self._iterator(self.val['_M_cmpts'])


+class StdPairPrinter:
+    "Print a std::pair object, with 'first' and 'second' as children"
+
+    def __init__(self, typename, val):
+        self.val = val
+
+    class _iter(Iterator):
+        "An iterator for std::pair types. Returns 'first' then 'second'."
+
+        def __init__(self, val):
+            self.val = val
+            self.which = 'first'
+
+        def __iter__(self):
+            return self
+
+        def __next__(self):
+            if self.which is None:
+                raise StopIteration
+            which = self.which
+            if which == 'first':
+                self.which = 'second'
+            else:
+                self.which = None
+            return (which, self.val[which])
+
+    def children(self):
+        return self._iter(self.val)
+
+    def to_string(self):
+        return None
+
+
# A "regular expression" printer which conforms to the
# "SubPrettyPrinter" protocol from gdb.printing.
class RxPrinter(object):
@@ -1629,6 +1662,7 @@ def build_libstdcxx_dictionary ():
    libstdcxx_printer.add_container('std::', 'map', StdMapPrinter)
    libstdcxx_printer.add_container('std::', 'multimap', StdMapPrinter)
    libstdcxx_printer.add_container('std::', 'multiset', StdSetPrinter)
+    libstdcxx_printer.add_version('std::', 'pair', StdPairPrinter)
    libstdcxx_printer.add_version('std::', 'priority_queue',
                                  StdStackOrQueuePrinter)
    libstdcxx_printer.add_version('std::', 'queue', StdStackOrQueuePrinter)
diff --git a/libstdc++-v3/testsuite/20_util/pair/86751.cc b/libstdc++-v3/testsuite/20_util/pair/86751.cc
new file mode 100644
index 00000000000..76a76c0d656
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/pair/86751.cc
@@ -0,0 +1,33 @@
+// Copyright (C) 2018 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do compile { target c++11 } }
+
+#include <utility>
+
+struct X {
+  template<typename T> operator T() const;
+};
+
+
+void
+test01()
+{
+  std::pair<int, int> p;
+  X x;
+  p = x;  // PR libstdc++/86751
+}
diff --git a/libstdc++-v3/testsuite/20_util/pair/ref_assign.cc b/libstdc++-v3/testsuite/20_util/pair/ref_assign.cc
new file mode 100644
index 00000000000..ea37fcfcf52
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/pair/ref_assign.cc
@@ -0,0 +1,74 @@
+// Copyright (C) 2018 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run { target c++11 } }
+
+#include <utility>
+#include <testsuite_hooks.h>
+
+void
+test01()
+{
+  typedef std::pair<int&, int> pair_type;
+  int i = 1;
+  int j = 2;
+  pair_type p(i, 3);
+  const pair_type q(j, 4);
+  p = q;
+  VERIFY( p.first == q.first );
+  VERIFY( p.second == q.second );
+  VERIFY( i == j );
+}
+
+void
+test02()
+{
+  typedef std::pair<int, int&> pair_type;
+  int i = 1;
+  int j = 2;
+  pair_type p(3, i);
+  const pair_type q(4, j);
+  p = q;
+  VERIFY( p.first == q.first );
+  VERIFY( p.second == q.second );
+  VERIFY( i == j );
+}
+
+void
+test03()
+{
+  typedef std::pair<int&, int&> pair_type;
+  int i = 1;
+  int j = 2;
+  int k = 3;
+  int l = 4;
+  pair_type p(i, j);
+  const pair_type q(k, l);
+  p = q;
+  VERIFY( p.first == q.first );
+  VERIFY( p.second == q.second );
+  VERIFY( i == k );
+  VERIFY( j == l );
+}
+
+int
+main()
+{
+  test01();
+  test02();
+  test03();
+}


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]