boostorg / algorithm

Boost.org algorithm module
http://boost.org/libs/algorithm
Boost Software License 1.0
114 stars 105 forks source link

[apply_permutation] Consistent interface, fix includes; use C++03. #42

Closed jeremy-murphy closed 5 years ago

jeremy-murphy commented 6 years ago

I love C++11 as much as everyone else but this component was mistakenly including the C++11 header <type_traits> when it should have included <iterator> (for std::iterator_traits). So the occasional auto and the using type alias don't appear to me to warrant requiring the newer language standard. Then, you know what it's like, you fix one thing and you can't stop.

So I also made the interface and documentation consistent with using 3 iterators, not 4. That makes sense to me since the fourth iterator is not actually used in the algorithm logic anyway. (Yes, I know lots of people are excited about adding a fourth iterator to the standard algorithms, etc, etc.)

I changed the unit test conventions a bit but it's no big deal to me to revert that change.

Also added assertions of valid, non-overlapping iterator range inputs.

mclow commented 6 years ago

I know lots of people are excited about adding a fourth iterator to the standard algorithms, etc, etc

One of those people is me.

jeremy-murphy commented 6 years ago

I know lots of people are excited about adding a fourth iterator to the standard algorithms, etc, etc

One of those people is me.

I can imagine, I know your involved with that kind of work. My comment was just meant to mean that even though that work is happening, I hope it doesn't mean that four iterators are expected even when the fourth is redundant.

mclow commented 6 years ago

There's always four iterators for this kind of thing. Sometimes it is implied, but that doesn't mean it's not there.

If you have four iterators, the implementation can (for example) check to see if there's enough room in the output range. (even though we don't do that at the moment).

jeremy-murphy commented 6 years ago

I get what you mean although I don't strictly agree, or I just don't agree with your terminology, but it's really tangential to this PR. Don't worry, I understand the utility that an explicit fourth iterator parameter can bring. I just hope that you agree (or can convince me otherwise), that there is no need to have it explicit for this algorithm where the two iterator range sizes must be equal.

jeremy-murphy commented 6 years ago

I don't want to let a quibble about iterators get in the way; is there something you would like me to change in this PR? Or do you want me to convince you that three is actually better than four in this case?

mclow commented 6 years ago

is there something you would like me to change in this PR?

If you remove the change in the interface of the algorithm - all the rest is fine.

jeremy-murphy commented 6 years ago

I see. OK, let's merge the stuff we agree on and come back to the interface later.

mclow commented 5 years ago

The "de-C++11"ing of this has already been done. I committed the test changes as 0a57ec3. There's nothing else to do here.