fix: Wrong return type of copy_fn and missing typedef in destruct_range

[marco-langer]

Description

This PR fixes some minor issues in algorithm.hpp:

• removed unnecessary non-const std::copy overload
• fixed wrong return type of copy_fn
• added missing typedef to destruct_range

Tasklist

• Ensure all CI builds pass
• Review and approve

[codecov]

Codecov Report

Merging #708 (7e87ecd) into develop (573ba13) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #708      +/-   ##
===========================================
- Coverage    81.10%   81.10%   -0.01%     
===========================================
  Files          117      117              
  Lines         5171     5170       -1     
===========================================
- Hits          4194     4193       -1     
  Misses         977      977              

[mloskot]

LGTM, thanks!

Is this still a draft?

[mloskot]

Hmm, amazing it remained unrevealed for long time.
How did you find it out?

[marco-langer]

Hmm, amazing it remained unrevealed for long time.

I was also surprised after realizing this bug has been there for at least 15 years. I guess the reason is copy_fn is not used in many places and as long as the output iterator is implicitly convertible to the input iterator (as it is the case for pointers which only differ in their const-qualifier), everything is fine.

How did you find it out?

Just me, the algorithm header and a cup of coffee.

[marco-langer]

Is this still a draft?

I wanted to think a second time about these core library changes before rushing them as a last minute change into the next release.

• Why didn't the missing typedef in destruct_range show up in the CI? Is this dead code? Should we add more test cases? Is this part, which deals with destructing non-trivial-destructible pixel types, some kind of academic over-engineering which has no real world application (otherwise there would have been already bug reports?)?
• Are those two std::copy specializations really redundant? And if yes, is specialization of std::copy for pixel types necessary at all? Shouldn't std::copy internally already employ a memmove strategy in case of trivial copyable value types? Wouldn't this be a better solution rather than this near-UB reinterpret_cast of the pixel types? Are the pixel types trivial copyable? If not, why not?

[mloskot]

I agree with your points @marco-langer
I also was wondering about the actual redundancy of the std::copy thing.
Let's postpone it after 1.80 is out - it's already too late for major changes (other than tweaking internal stuff like CI or CMake configs).