[Bug libstdc++/52433] New: [C++11] debug mode iterators need move constructors

[Bug libstdc++/52433] New: [C++11] debug mode iterators need move constructors

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52433

             Bug #: 52433
           Summary: [C++11] debug mode iterators need move constructors
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Keywords: rejects-valid
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: redi@gcc.gnu.org


#define _GLIBCXX_DEBUG
#include <vector>

struct X {
    X() = default;
    X(const X&) = default;
    X(X&&) = default;
    std::vector<int>::iterator i;
};

X f()
{
    X x;
    return x;
}

t.cc: In function 'X f()':
t.cc:14:12: error: use of deleted function 'X::X(X&&)'
t.cc:7:5: note: 'X::X(X&&)' is implicitly deleted because the default
definition would be ill-formed:
t.cc:7:5: error: non-static data member 'X::i' does not have a move constructor
or trivial copy constructor


Untested patch:

--- include/debug/safe_iterator.h.orig  2012-02-29 12:42:45.876490264 +0000
+++ include/debug/safe_iterator.h       2012-02-29 12:44:57.535918988 +0000
@@ -169,6 +169,19 @@
                              ._M_iterator(__x, "other"));
       }

+      /// @post @p __x is singular and unattached
+      _Safe_iterator(_Safe_iterator&& __x)
+      {
+       // _GLIBCXX_RESOLVE_LIB_DEFECTS
+       // DR 408. Is vector<reverse_iterator<char*> > forbidden?
+       _GLIBCXX_DEBUG_VERIFY(!__x._M_singular()
+                             || __x._M_current == _Iterator(),
+                             _M_message(__msg_init_copy_singular)
+                             ._M_iterator(*this, "this")
+                             ._M_iterator(__x, "other"));
+        swap(*this, __x);
+      }
+
       /**
        *  @brief Converting constructor from a mutable iterator to a
        *  constant iterator.

[Bug libstdc++/52433] [C++11] debug mode iterators need to move

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52433

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[C++11] debug mode          |[C++11] debug mode
                   |iterators need move         |iterators need to move
                   |constructors                |

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-02-29 12:58:04 UTC ---
Also needs a move-assignment operator:

#define _GLIBCXX_DEBUG
#include <vector>

struct X {
    X() = default;
    X(const X&) = default;
    X(X&&) = default;
    X& operator=(X&&) = default;
    std::vector<int>::iterator i;
};

X f()
{
    X x;
    x = X();
    return x;
}

[Bug libstdc++/52433] [C++11] debug mode iterators need to move

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52433

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-02-29 13:01:25 UTC ---
Also untested, and sub-optimal (swapping would be better than copying):

      /**
       * @brief Move assignment.
       * @post @p __x is singular and unattached
       */
      _Safe_iterator&
      operator=(_Safe_iterator&& __x)
      {
    _Safe_iterator __tmp(std::move(__x));
        *this = __tmp;
        return *this;
      }

[Bug libstdc++/52433] [C++11] debug mode iterators need to move

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52433

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot       |redi at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-02-29 14:33:02 UTC ---
(In reply to comment #0)
> 
> +      /// @post @p __x is singular and unattached
> +      _Safe_iterator(_Safe_iterator&& __x)
> +      {
> +       // _GLIBCXX_RESOLVE_LIB_DEFECTS
> +       // DR 408. Is vector<reverse_iterator<char*> > forbidden?
> +       _GLIBCXX_DEBUG_VERIFY(!__x._M_singular()
> +                             || __x._M_current == _Iterator(),
> +                             _M_message(__msg_init_copy_singular)
> +                             ._M_iterator(*this, "this")
> +                             ._M_iterator(__x, "other"));
> +        swap(*this, __x);

Doh, that will recursively call the move constructor.

We could do:

      /// @post @p __x is singular and unattached
      _Safe_iterator(_Safe_iterator&& __x)
      {
    *this = __x;
    _Safe_iterator __singular;
    __x = __singular;
      }

but it would be more efficient to implement a swap() member and use that for
both move construction and move-assignment

[Bug libstdc++/52433] [C++11] debug mode iterators need to move

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52433

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-02-29
     Ever Confirmed|0                           |1

--- Comment #4 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-02-29 15:35:05 UTC ---
Thanks for handling this. Yesterday was looking into the debug-mode bits of the
vector<bool> swap issue and noticed something quite weird going on ;)

[Bug libstdc++/52433] [C++11] debug mode iterators need to move

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52433

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-02-29 16:37:43 UTC ---
The problem exists in all active branches and isn't a regression, so I'll
implement a swap asap but it isn't urgent for 4.7.0

[Bug libstdc++/52433] [C++11] debug mode iterators need to move

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52433

--- Comment #6 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-02-29 17:01:44 UTC ---
Sure, sure, likewise for vector<bool> swap ;)

[Bug libstdc++/52433] [C++11] debug mode iterators need to move

[daniel.kruegler at googlemail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52433

Daniel Krügler <daniel.kruegler at googlemail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |daniel.kruegler at
                   |                            |googlemail dot com

--- Comment #7 from Daniel Krügler <daniel.kruegler at googlemail dot com> 2012-03-01 06:49:30 UTC ---
One could argue that this issue is due to

http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#1402

If the current P/R becomes accepted, the code would become accepted, too ;-)
Joke aside: Would it be useful to connect this issue with CWG 1402?

[Bug libstdc++/52433] [C++11] debug mode iterators need to move

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52433

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-03-01 09:35:41 UTC ---
Ah yes, thanks for the link.

(In reply to comment #7)
> Joke aside: Would it be useful to connect this issue with CWG 1402?

I think you just did, with that link :)

I'm still going to add move ops to the _Safe_iterator class template because if
the P/R of 1402 is accepted the change is unlikely to be applied to the older
release branches

[Bug libstdc++/52433] [C++11] debug mode iterators need to move

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52433

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-03-04 12:49:27 UTC ---
Author: redi
Date: Sun Mar  4 12:49:22 2012
New Revision: 184880

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=184880
Log:
        PR libstdc++/52433
        * include/debug/safe_iterator.h (_Safe_iterator): Add move
        constructor and move assignment operator.
        * testsuite/23_containers/vector/debug/52433.cc: New.

Added:
    trunk/libstdc++-v3/testsuite/23_containers/vector/debug/52433.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/debug/safe_iterator.h

[Bug libstdc++/52433] [C++11] debug mode iterators need to move

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52433

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |4.8.0
   Target Milestone|---                         |4.6.4

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-03-04 12:59:33 UTC ---
Fixed on trunk so far.

I realised swapping isn't actually very simple, because the iterators need to
be unlinked/relinked into their container's lists so I just implemented it with
_M_attach and _M_detach instead.

[Bug libstdc++/52433] [C++11] debug mode iterators need to move

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52433

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-03-08 01:05:07 UTC ---
Author: redi
Date: Thu Mar  8 01:05:01 2012
New Revision: 185089

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=185089
Log:
    PR libstdc++/52433
    * include/debug/safe_iterator.h (_Safe_iterator): Add debug checks
    to move constructor and move assignment operator.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/debug/safe_iterator.h

[Bug libstdc++/52433] [C++11] debug mode iterators need to move

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52433

--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-03-08 22:31:24 UTC ---
Author: redi
Date: Thu Mar  8 22:31:19 2012
New Revision: 185114

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=185114
Log:
    PR libstdc++/52433
    * include/debug/safe_iterator.h (_Safe_iterator): Add move
    constructor and move assignment operator.
    * testsuite/23_containers/vector/debug/52433.cc: New.

Added:
   
branches/gcc-4_6-branch/libstdc++-v3/testsuite/23_containers/vector/debug/52433.cc
Modified:
    branches/gcc-4_6-branch/libstdc++-v3/ChangeLog
    branches/gcc-4_6-branch/libstdc++-v3/include/debug/safe_iterator.h

[Bug libstdc++/52433] [C++11] debug mode iterators need to move

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52433

--- Comment #13 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-03-23 00:02:58 UTC ---
Author: redi
Date: Fri Mar 23 00:02:47 2012
New Revision: 185717

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=185717
Log:
    PR libstdc++/52433
    * include/debug/safe_iterator.h (_Safe_iterator): Add move
    constructor and move assignment operator.
    * testsuite/23_containers/vector/debug/52433.cc: New.

Added:
   
branches/gcc-4_7-branch/libstdc++-v3/testsuite/23_containers/vector/debug/52433.cc
Modified:
    branches/gcc-4_7-branch/libstdc++-v3/ChangeLog
    branches/gcc-4_7-branch/libstdc++-v3/include/debug/safe_iterator.h

[Bug libstdc++/52433] [C++11] debug mode iterators need to move

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52433

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
      Known to work|                            |4.7.1
         Resolution|                            |FIXED

--- Comment #14 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-03-23 00:04:23 UTC ---
fixed on trunk and 4.6 and 4.7 branches