gmane.comp.gcc.libstdc++.devel

Re: Fix stable_sort to work on iterators returning rvalue

Paolo Carlini — 2012-05-25T22:10:37

Patch looks pretty good to me. It would be great if the usual friends of 
libstdc++ could also have a look, in particular Chris (if I remember 
correctly he invented this stuff)

Thanks,
Paolo.

PS: as a very minor nit, remember the bool test variable in test01.

Fix stable_sort to work on iterators returning rvalue

François Dumont — 2012-05-25T20:42:03

Hi

     The issues I had when trying to use stable_sort with move_iterator 
was in fact not all coming from a missing swap overload. In fact 
stable_sort do not accept iterators with dereference operator returning 
pure rvalue. I use std::vector<bool>::iterator to illustrate this 
problem. The problem was in fact coming from 
__uninitialized_construct_buf that was taking a lvalue reference. I 
simply modify the function and the underlying helper struct to 
dereference the iterator as late as possible that is to say only when we 
need to pass it to the std::move function.

2012-05-25  François Dumont <fdumont< at >gcc.gnu.org>

     * include/bits/stl_tempbuf.h (__uninitialized_construct_buf)
     (__uninitialized_construct_buf_dispatch<>::__ucr): Fix to work
     with iterator returning rvalue.
     * testsuite/25_algorithms/stable_sort/3.cc: New.

Tested under x86_64 linux with make check and make CXXFLAGS=-std=c++11 
check.

Ok to commit ?

François

Re: iter_swap on move_iterator

Christopher Jefferson — 2012-05-19T08:01:39

Furthermore, we used to use move_iterator internally in std::sort, and 
had to take it out because it would sometimes lead to values being lost, 
if the type had a 'by value' operator< like:

bool operator<(MyType t1, MyType t2) { ... }

Because as soon as we compared we would move the value into the 
comparitor and it would be lost.

I would personally advise against supporting move_iterator in the 
standard algorithms, unless you are also prepared to make it safe in 
such situations (which might itself be impossible, without breaking 
other things).

In short, move_iterator is in my opinion easily misused and not a proper 
iterator and not compiling it might be a 'feature' :)

Chris

Re: iter_swap on move_iterator

Jonathan Wakely — 2012-05-19T00:07:35

I agree, and will just add that support for swapping rvalues of
standard library classes was added to a C++0x draft but then
intentionally removed again as undesirable and IIRC in some cases
unsafe.

Re: iter_swap on move_iterator

Daniel Krügler — 2012-05-18T18:12:47

2012/5/18 François Dumont <frs.dumont< at >gmail.com>:

I don't think so. The library requirements on the iterator type is
ValueSwappable, which
means that "for any dereferenceable object x of type X, *x is
swappable". So, unless
there exists active rvalue-swap support provided by user-provided value type of
the wrapped iterator of std::move_iterator (or by the reference type
of the wrapped
iterator, once LWG 2106 becomes accepted), the library implementation should
not interfere here.

- Daniel

Re: iter_swap on move_iterator

François Dumont — 2012-05-18T12:21:45

Ok for how to have iter_swap work with move_iterator, users must grant 
the necessary swap overload. But what about the other algos like 
stable_sort I have tested at first ? Shouldn't they be adapted to work 
with move_iterator ?
Agree but the compiler message do not show the '&&', this is especially 
obvious in:

/home/fdt/dev/gcc-build/x86_64-unknown-linux-gnu/libstdc++-v3/include/bits/move.h:166:5: 
note:   no known conversion for argument 1 from 
'std::move_iterator<greedy_ops::X*>::value_type {aka greedy_ops::X}' to 
'greedy_ops::X&'

The compiler is perfectly able to take a lvalue reference to a 
value_type so this message makes no sens. If it was saying 
"...::reference {aka greedy_ops::X&&}' to ..." it would be clearer.

François

Re: iter_swap on move_iterator

Daniel Krügler — 2012-05-17T18:47:00

2012/5/17 François Dumont <frs.dumont< at >gmail.com>:

Yes, when the referenced type (or the iterator's reference) provides a
swap overload
that accepts rvalues. Remember that move_iterator will produce an rvalue
on dereference, this rvalue is provided to the swap overload that is found
by ADL. Given your description I would expect that the test type does not
provide a swap overload accepting rvalues, thus the std::swap is selected,
but cannot accept the arguments.


Yes, but move_iterator<>::reference is value_type&&.

- Daniel

iter_swap on move_iterator

François Dumont — 2012-05-17T17:53:55

Hi

     While working on removal of code duplications in algos 
implementation I challenge them with move_iterator. I started with 
stable_sort and it doesn't compile. I summarize the problem to the fact 
that iter_swap can't be used on move_iterator. As, in C++11, iter_swap 
simply forward to swap(*__a, *__b) it is normal that it can't accept 
rvalue but shouldn't iter_swap be adapted to accept move_iterator ?

     I also find the compilation error message surprising. I have added 
a call to iter_swap in 24_iterators/move_iterator/greedy_ops.cc and the 
error message is:

In file included from 
/home/fdt/dev/gcc-build/x86_64-unknown-linux-gnu/libstdc++-v3/include/bits/char_traits.h:41:0, 

                  from 
/home/fdt/dev/gcc-build/x86_64-unknown-linux-gnu/libstdc++-v3/include/ios:41, 

                  from 
/home/fdt/dev/gcc-build/x86_64-unknown-linux-gnu/libstdc++-v3/include/ostream:40, 

                  from 
/home/fdt/dev/gcc-build/x86_64-unknown-linux-gnu/libstdc++-v3/include/iterator:64,

Re: [v3] fix libstdc++/53263

Paolo Carlini — 2012-05-15T20:41:26

Thanks!

Paolo.

Re: [v3] fix libstdc++/53263

François Dumont — 2012-05-15T20:30:56

Details added

2012-05-11  François Dumont <fdumont< at >gcc.gnu.org>

     PR libstdc++/53263
     * include/debug/safe_iterator.h (__gnu_debug::__base): Move...
     * include/debug/functions.h: ... Here.
     (__check_sorted_aux): Add overload for random access safe
     iterators.
     (__check_partition_lower_aux): New.
     (__check_partition_lower): Use latter.
     (__check_partition_upper_aux): New.
     (__check_partition_upper): Use latter.
     * include/debug/macros.h (__glibcxx_check_heap)
     (__glibcxx_check_heap_pred): Use __gnu_debug::__base on iterator range.

François

On 05/14/2012 09:55 PM, Paolo Carlini wrote:

Re: Remove algo code duplication

Marc Glisse — 2012-05-14T20:02:47


Or instead of using directly a functor on the values, one could write the 
algorithm using a functor on iterators, which would take a and b and 
return either *a<*b or cmp(*a,*b). Maybe not so nice.

Re: [v3] fix libstdc++/53263

Paolo Carlini — 2012-05-14T19:55:16

What does it mean "Add debug function overloads"? Which are the *names* 
of those overloads? We want to know that, because we want to be able to 
grep the ChangeLog and see exactly when somebody touched each and every 
functions.

Paolo.

Re: [v3] fix libstdc++/53263

François Dumont — 2012-05-14T19:53:03

I don't think I miss any file, tell me otherwise.

My bugzilla account is fully functional now.

Thanks

François

Re: [v3] fix libstdc++/53263

Paolo Carlini — 2012-05-12T21:44:35

Weird. Thanks a lot Ian. Francois, please double check that now you are Ok.

Thanks,
Paolo.

Re: [v3] fix libstdc++/53263

Ian Lance Taylor — 2012-05-12T05:59:04

[ Adding overseers to make a record of this ]


Jonathan Wakely <jwakely.gcc< at >gmail.com> writes:


Anybody < at >gcc.gnu.org is supposed to get various permissions in bugzilla.
I have no idea why these were not turned on for fdumont< at >gcc.gnu.org.  I
just turned them on.

Ian

Re: [v3] fix libstdc++/53263

Jonathan Wakely — 2012-05-11T23:28:13

Oops, Paolo pointed out I didn't actually CC Ian... done this time!
Ian, see above, we'd be grateful for your assistance.

Re: [v3] fix libstdc++/53263

Jonathan Wakely — 2012-05-11T23:08:06

My understanding is that any < at >gcc.gnu.org account should have full
permissions to update and close any bugs.

I'v CC'd Ian as I think he can view and edit any user's bugzilla
permissions and might know why fdumont< at >gcc doesn't have the full set
of permissions that a gcc.gnu.org account usually has, or at least
could set them.

Re: [v3] fix libstdc++/53263

Paolo Carlini — 2012-05-11T22:46:01

Note, as a rule the ChangeLog should explicitly mention all the files 
you are touching. If you can, please amend it. Thanks!
Didn't Jon provide some guidance about this? Jon?

Thanks,
Paolo.

Re: [v3] fix libstdc++/53263

Paolo Carlini — 2012-05-10T21:18:51

Hi,

On 05/09/2012 11:02 PM, François Dumont wrote:

Thanks. Considering that this isn't a regression and also that nobody 
reported the issue for so many years, the patch seems a bit largish to 
me to go into the branch. Thus, let's apply to mainline only and 
consider the issue closed. If people insist, seriously insist ;) we may 
reconsider for 4.7.2.

Thanks again!
Paolo.

PS: are you finally able to manage Bugzilla, yes?

Re: h8300-elf build broken

DJ Delorie — 2012-05-10T21:07:10


Done!

Re: [Mingw-w64-public] Thoughts on supporting the C++11 thread library on Windows

Jonathan Wakely — 2012-05-10T14:52:26

Yep, if you don't explicitly configure with --enable-threads then the
configure script picks a suitable default, which is "win32" on
Windows.



Yes, I don't think we have an open bugzilla report about that
portability problem, but it's a known issue.


Have those changes come back upstream? It sounds as though they should do.



That makes sense for <thread> because the standard is the
authoritative source.  For GCC's configuration the GCC sources are the
authoritative source :-)

Sorry if my last mail seemed impatient, I do appreciate your feedback
based on your own experience of implementing <thread>.