[Bug libstdc++/47628] New: non-compliant C++0x erase methods on STL containers

[Bug libstdc++/47628] New: non-compliant C++0x erase methods on STL containers

[blelbach at cct dot lsu.edu]

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

           Summary: non-compliant C++0x erase methods on STL containers
           Product: gcc
           Version: 4.6.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: blelbach@cct.lsu.edu


Apologizes if this has changed in the draft standard (my copy of n3092
indicates it hasn't):

As of r169874, std::set<>, std::map<>, std::multiset<>, std::multimap<>,
std::unordered_set<>, std::unordered_map<>, std::unordered_multiset<> and
std::unordered_multimap<> have C++0x erase methods; instead of taking an
`iterator' (or a range of `iterators' for some of the aforementioned
containers), the GNU implementation of these containers accept a
`const_iterator'. In ISO/IEC N3092, the specification of the erase() method is
in [associative.reqmts] (Section 23.2.4) table 99 and [unord.req] (Section
23.2.5) table 100.

I do not believe this problem affects the library in C++03/C++99 mode, as a
preprocessor macro selects the old C++99/C++03 version of erase(). This is
problematic in particular for std::map<>, where it can cause a call to erase()
to be ambigious. I noticed this problem when compiling the Boost.Signals
library (I'm a Boost developer) with a freshly built GCC 4.6.0.

Please find attached a patch against trunk that fixes this problem.

[Bug libstdc++/47628] non-compliant C++0x erase methods on STL containers

[blelbach at cct dot lsu.edu]

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

--- Comment #1 from Bryce Lelbach (wash) <blelbach at cct dot lsu.edu> 2011-02-07 01:00:28 UTC ---
Created attachment 23262
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=23262
Fix for STL erase() methods on associative containers

[Bug libstdc++/47628] non-compliant C++0x erase methods on STL containers

[redi at gcc dot gnu.org]

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

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-02-07 09:17:08 UTC ---
known issue, the fix changes the ABI and so can't be done yet

N.B. N3225 is the current draft

[Bug libstdc++/47628] non-compliant C++0x erase methods on STL containers

[redi at gcc dot gnu.org]

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

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-02-07 09:30:42 UTC ---
Oops, I think I'm confusing this with another related issue - despite what I
said we do seem to have changed the signature, and we currently implement the
C++0x draft spec.

Tables 99 and 100 say erase takes a const_iterator, so does the synopsis of
each container

[Bug libstdc++/47628] non-compliant C++0x erase methods on STL containers

[paolo.carlini at oracle dot com]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID

--- Comment #4 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-02-07 10:58:20 UTC ---
Indeed, in mainline, would be 4.6.0, we are already doing the right thing vs
N3225, there is nothing to fix here.

[Bug libstdc++/47628] non-compliant C++0x erase methods on STL containers

[blelbach at cct dot lsu.edu]

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

--- Comment #5 from Bryce Lelbach (wash) <blelbach at cct dot lsu.edu> 2011-02-07 14:20:09 UTC ---
Sorry! Wasn't aware this was correct.

[Bug libstdc++/47628] non-compliant C++0x erase methods on STL containers

[redi at gcc dot gnu.org]

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

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-02-07 17:03:01 UTC ---
btw, your analysis at
http://article.gmane.org/gmane.comp.lib.boost.devel/214412 is bogus

There is no C++99, presumably you mean C++98

C++0x mode is absolutely not the default mode of either g++ or libstdc++, I
don't know what gave you that idea.

The Boost.Signals code is ambiguous with or without C++0x mode enabled,
although there is an extra ambiguity with C++0x mode

I believe the underlying problem is that stored_group has a "catch all"
constructor, which means that the conversion from non-const iterator to
const_iterator is not better than the conversion from non-const iterator to
key_type (i.e. stored_group)

You should try this (untested) alternative fix, which should work for C++03 and
C++0x, and looks like an improvement anyway:

--- boost/signals/detail/named_slot_map.hpp.orig        2011-02-07
17:01:46.297942798 +0000
+++ boost/signals/detail/named_slot_map.hpp     2011-02-07 17:01:47.665572465
+0000
@@ -35,7 +35,7 @@
   stored_group(storage_kind kind = sk_empty) : kind(kind), group() { }

   template<typename T>
-  stored_group(const T& group) : kind(sk_group), group(new T(group)) { }
+  explicit stored_group(const T& group) : kind(sk_group), group(new T(group))
{ }

   bool is_front() const { return kind == sk_front; }
   bool is_back() const { return kind == sk_back; }

[Bug libstdc++/47628] non-compliant C++0x erase methods on STL containers

[redi at gcc dot gnu.org]

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

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-02-07 17:17:54 UTC ---
Here's a reduced form of the code, which works with GCC 4.5 and earlier, but is
ambiguous with 4.6

#include <map>

struct Key
{
    Key() { }

    Key(const Key&) { }

    template<typename T>
        Key(const T&)
        { }

    bool operator<(const Key&) const;
};

typedef std::map<Key, int> Map;

void f()
{
    Map m;
    (void) m[Key()];
    Map::iterator i = m.begin();
    m.erase(i);
}

I'm not sure if the code is valid or not (Paolo?)

In C++0x mode the ambiguity is in the user code, because non-const iterator can
be converted to const_iterator or key_type (I believe this behaviour is
required by the standard)

In C++98 mode it's in the library code when calling _M_t.erase - this is a
regression.  Paolo, should _Rb_tree::erase take a non-const iterator in c++98
mode?  That's what map::erase passes it.

[Bug libstdc++/47628] non-compliant C++0x erase methods on STL containers

[paolo.carlini at oracle dot com]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jwakely.gcc at gmail dot
                   |                            |com

--- Comment #8 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-02-07 17:49:33 UTC ---
Boring. Time ago I removed the other erase overloads for C++98, because it
seemed redundant. Now, I'm thinking, instead of readding it, with all the
stupid reduncancy, can't we just have a single overload taking a const_iterator
and avoid differently the ambiguity with _Rb_tree::erase(const key_type&)? Like
renaming it, whatever?

[Bug libstdc++/47628] non-compliant C++0x erase methods on STL containers

[paolo.carlini at oracle dot com]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
   Last reconfirmed|                            |2011.02.07 17:56:37
         Resolution|INVALID                     |
     Ever Confirmed|0                           |1

--- Comment #9 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-02-07 17:56:37 UTC ---
Well, thanks to the new _M_erase_aux helper I added at that time, it's actually
just two lines of code, let's play safe and do that, for 4.6.0. Thanks Jon for
the testcase.

[Bug libstdc++/47628] non-compliant C++0x erase methods on STL containers

[paolo.carlini at oracle dot com]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot       |paolo.carlini at oracle dot
                   |gnu.org                     |com
   Target Milestone|---                         |4.6.0

[Bug libstdc++/47628] non-compliant C++0x erase methods on STL containers

[redi at gcc dot gnu.org]

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

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-02-07 18:00:50 UTC ---
Great.

For the record, Bryce, the change to the standard was done by DR 180
http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#180
see also
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2350.pdf

The code is invalid in C++0x, so I recommend adding the 'explicit' keyword to
the template constructor, if the Boost testsuite passes with that change.  That
will allow the same code to work with C++03 and C++0x compilers.

[Bug libstdc++/47628] non-compliant C++0x erase methods on STL containers

[paolo at gcc dot gnu.org]

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

--- Comment #11 from paolo at gcc dot gnu.org <paolo at gcc dot gnu.org> 2011-02-07 20:15:59 UTC ---
Author: paolo
Date: Mon Feb  7 20:15:48 2011
New Revision: 169899

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=169899
Log:
2011-02-07  Paolo Carlini  <paolo.carlini@oracle.com>

    PR libstdc++/47628
    * include/bits/stl_tree.h (_Rb_tree::erase(iterator), erase(iterator,
    iterator)): Add back in C++03 mode.
    * testsuite/23_containers/map/modifiers/erase/47628.cc: New.
    * testsuite/23_containers/multimap/modifiers/erase/47628.cc: Likewise.

Added:
    trunk/libstdc++-v3/testsuite/23_containers/map/modifiers/erase/
    trunk/libstdc++-v3/testsuite/23_containers/map/modifiers/erase/47628.cc
    trunk/libstdc++-v3/testsuite/23_containers/multimap/modifiers/erase/
   
trunk/libstdc++-v3/testsuite/23_containers/multimap/modifiers/erase/47628.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/stl_tree.h

[Bug libstdc++/47628] non-compliant C++0x erase methods on STL containers

[paolo.carlini at oracle dot com]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #12 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-02-07 20:18:46 UTC ---
Done.

[Bug libstdc++/47628] non-compliant C++0x erase methods on STL containers

[redi at gcc dot gnu.org]

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

--- Comment #13 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-02-23 13:41:24 UTC ---
any reason the change didn't make it into Boost 1.46.0 ?