Re: Remove algo code duplication

[François Dumont <frs dot dumont at gmail dot com>]

On 04/28/2012 01:28 PM, Marc Glisse wrote:
> 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<.
non-template, really ?
> > 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.
You mean functor copies, right ? I expect the compiler to optimize them 
away even if there are options to forbid him to do so. In fact I also 
expect the compiler to optimize away all the functor simple plumbery, if 
there is a way to make this assumption safer do not hesitate to tell me. 
I wonder if not qualifying the parenthesis operators as const might be a 
reason for the compiler to not inline it. If it is so important I will 
try to detect if the invoked operator is const qualified to make the 
parenthesis operator const also. Maybe I should add a noexcept 
qualification ?
> > 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.
In addition to the reason below I also prefer to see concept checks 
performed as near as possible in the call-stack to the Standard algo 
called by user code to guaranty the cleanest diagnostic as possible even 
at code duplication cost in this case.
> > 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?
Agree.

François