This is the mail archive of the
mailing list for the libstdc++ project.
Re: Fix stable_sort to work on iterators returning rvalue
- From: François Dumont <frs dot dumont at gmail dot com>
- To: "libstdc++ at gcc dot gnu dot org" <libstdc++ at gcc dot gnu dot org>
- Date: Mon, 28 May 2012 21:00:47 +0200
- Subject: Re: Fix stable_sort to work on iterators returning rvalue
- References: <4FC36F8F.firstname.lastname@example.org>
On 05/28/2012 12:11 PM, Christopher Jefferson wrote:
My main concern is one I have mentioned previously. I'm unsure all our
code works with things like move_iterator, even when it correctly
compiles. We previously took out internal uses of move_iterator,
because while the code compiled it did not work correctly. Look at bug
: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48038 for any example.
With this change, code which uses move_iterator, and operator< which
pass by value, will cause values to be 'eaten' during the sort. Now,
the standard in my opinion doesn't say this would be a bug, but it
certainly seems like it would be unpleasant.
How to fix this? The simplest way might be to wrap all predicates /
operator< in a wrapper which just takes lvalue references. Not sure if
it is worth going to that much pain just to fix this.
At the very least, I would want to see a bunch of tests which make
sure we don't have any other code which accidentally 'eats' users
data. Make sure you catch all the various bits of sort, some of which
get triggered rarely. I realise that is putting other people to a
higher standard than I put myself, but we have learnt this is a tricky
Does it mean that you refuse the patch ?
The patch purpose is not to make the code compatible with
move_iterator but to make it compatible with iterator types that return
pure rvalue through their dereference operator. As a side effect
std::move_iterator are going to compile too which might be bad but is it
really a reason to forbid other kind of iterators ?
Of course we should find a good way to handle move_iterator, and I
can spend some time on it, but I think that it should be the subject of
a dedicated patch.