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

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.10%. Comparing base (573ba13) to head (7e87ecd).
⚠️ Report is 114 commits behind head on develop.

Additional details and impacted files

@@ 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 

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mloskot]

LGTM, thanks!

Is this still a draft?

[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).

[sdebionne]

I had this PR checked:

template <typename I, typename O> struct copy_fn {
BOOST_FORCEINLINE I operator()(I first, I last, O dst) const { return std::copy(first,last,dst); }
};

std::copy(first, last, dst) returns an output iterator of type O (one-past-the-last-written element in the destination), but the function's declared/returned type is I — the input iterator type. That's backwards: operator() should return O, not I.

Concretely this is used at algorithm.hpp:181cfor planar pixel copies:

static_for_each(first,last,dst,boost::gil::detail::copy_fn<IC1,IC2>());

where IC1 (source channel iterator) and IC2 (destination channel iterator) can be distinct types. Returning I instead of O either fails to compile when O isn't convertible to I, or silently returns the wrong iterator (converted back to the source type) when it happens to compile — losing the correct destination-advance information a caller would expect from a copy-like functor.

Dropping the redundant const-pointer std::copy overload nearby and fixes a missing value_t typedef in destruct_range_impl is correct.