Re: std::vector move assign patch

[Marc Glisse <marc dot glisse at inria dot fr>]

On Thu, 9 Jan 2014, Jonathan Wakely wrote:

> On 9 January 2014 12:22, H.J. Lu wrote:
> > On Fri, Dec 27, 2013 at 10:27 AM, François Dumont <frs.dumont@gmail.com> wrote:
> > > Hi
> > >
> > >     Here is a patch to fix an issue in normal mode during the move
> > > assignment. The destination vector allocator instance is moved too during
> > > the assignment which is wrong.
> > >
> > >     As I discover this problem while working on issues with management of
> > > safe iterators during move operations this patch also fix those issues in
> > > the debug mode for the vector container. Fixes for other containers in debug
> > > mode will come later.
> > >
> > > 2013-12-27  François Dumont <fdumont@gcc.gnu.org>
> > >
> > >     * include/bits/stl_vector.h (std::vector<>::_M_move_assign): Pass
> > >     *this allocator instance when building temporary vector instance
> > >     so that *this allocator do not get moved.
> > >     * include/debug/safe_base.h
> > >     (_Safe_sequence_base(_Safe_sequence_base&&)): New.
> > >     * include/debug/vector (__gnu_debug::vector<>(vector&&)): Use
> > >     latter.
> > >     (__gnu_debug::vector<>(vector&&, const allocator_type&)): Swap
> > >     safe iterators if the instance is moved.
> > >     (__gnu_debug::vector<>::operator=(vector&&)): Likewise.
> > >     * testsuite/23_containers/vector/allocator/move.cc (test01): Add
> > >     check on a vector iterator.
> > >     * testsuite/23_containers/vector/allocator/move_assign.cc
> > >     (test02): Likewise.
> > >     (test03): New, test with a non-propagating allocator.
> > >     * testsuite/23_containers/vector/debug/move_assign_neg.cc: New.
> > >
> > > Tested under Linux x86_64 normal and debug modes.
> > >
> > > I will be in vacation for a week starting today so if you want to apply it
> > > quickly do not hesitate to do it yourself.
> >
> > This caused:
> >
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59738
>
> Fixed by the attached patch, tested x86_64-linux and committed to
> trunk.  I've also rotated the libstdc++ ChangeLog.
>
>
> 2014-01-09  Jonathan Wakely  <jwakely@redhat.com>
>
>        PR libstdc++/59738
>        * include/bits/stl_vector.h (vector<>::_M_move_assign): Restore
>        support for non-Movable types.

It seems that this patch had 2 consequences that may or may not have been 
planned. Consider this example (from PR64601)

#include <vector>
typedef std::vector<int> V;
void f(V&v,V&w){ V(std::move(w)).swap(v); }
void g(V&v,V&w){ v=std::move(w); }

1) We generate shorter code for f than for g, probably since the fix for 
PR59738. g ends up zeroing v, copying w to v, and finally zeroing w, and 
for weird reasons (and because we swap the members one by one) the 
standard prevents us from assuming that v and w do not overlap in weird 
ways so we cannot optimize as much as one might expect.

2) g(v,v) seems to turn v into a nice empty vector, while f(v,v) turns it 
into an invalid vector pointing at released memory.

Since 2) is a nice side-effect, it may not be worth rewriting operator= in 
a way that improves 1) but loses 2). Anyway, just mentioning this here.

--
Marc Glisse