This is the mail archive of the mailing list for the libstdc++ project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Remove algo code duplication

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?


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]