Re: Remove algo code duplication

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

On Sat, 28 Apr 2012, François Dumont wrote:

>    Here is an other attempt to remove duplication of code in implementation 
> of Standard algos. There are several enhancements on the last proposal:

Hello, a few comments/questions because I am intested (don't take it as me 
trying to review your patch...).

> 1. No usage of C++11 lambdas anymore, code works fine in C++98 and code has 
> been really deleted this time. About 700 lines removed in stl_algo.h for 
> instance.

Ah, yes, to remove the code, the new solution has to work in C++98, which 
prevents the simpler solution of having a non-template functor __less that 
perfectly forwards anything it is given to operator<.

> 2. Compatible with existing code, code submitted by Christopher last time 
> will still compile. To do so I used std::iterator_traits<>::reference type so 
> that if the iterator is not const then the operator do not have to take const 
> reference or to be const qualified neither. For the same reason I have also 
> avoided some const qualifiers on the introduced functors.

It probably doesn't matter, but the difference with the current code can 
be seen. With -fno-elide-constructors you perform copies, operator< is 
always given an lvalue, etc.

> In libstdcxx_so_7 branch the algo version using operators was calling the 
> version taking a functor. The result was that concept checks on the operator 
> version was performed twice. It is not a big concern when it is really pure 
> concept checks but in trunk there is also the debug checks that can be quite 
> expensive, checking that a range is sorted for instance. So I prefer to 
> introduce an internal implementation that contains only the algo 
> implementation without any concept checks and have it call from the Standard 
> algos.

Can't you just remove the checks from the operator version, in that case? 
It also avoids code duplication.

> Those pure implementations are also the one used for internal purpose.

Ah, makes sense then to have an unchecked version.

>    * include/bits/predefined_ops.h: New.

A name like __gnu_cxx::__ops::less<Iter> that designates a comparison on 
dereferenced iterators is quite confusing as it differs from std::less. 
Maybe with "iterator" (or just "iter") in the name of the namespace (or 
the functor) it would help?

Thanks for removing code,

--
Marc Glisse