boostorg / gil

Boost.GIL - Generic Image Library | Requires C++14 since Boost 1.80
https://boostorg.github.io/gil
Boost Software License 1.0
178 stars 163 forks source link

feat: Added for_each_pixel overload for any_image #648

Closed marco-langer closed 2 years ago

marco-langer commented 2 years ago

Description

Fixes #579

References

Tasklist

marco-langer commented 2 years ago

The suggested solution in the issue does not work, because we cannot use generic lambdas in C++11.

I've marked my PR as WIP, because I am not quite sure whether it is a good idea to take the address of the callable and store a pointer to it in the helper function object. I could also refactor it to take it by value and move it into the helper functor. Any options on this?

codecov[bot] commented 2 years ago

Codecov Report

Merging #648 (fc88042) into develop (caf92fa) will increase coverage by 0.02%. The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #648      +/-   ##
===========================================
+ Coverage    78.77%   78.79%   +0.02%     
===========================================
  Files          117      117              
  Lines         5031     5036       +5     
===========================================
+ Hits          3963     3968       +5     
  Misses        1068     1068              
mloskot commented 2 years ago

@marco-langer Thank you for the PR. Storing the functor directly, by value, is fine.

mloskot commented 2 years ago

What's the status of this PR? FYI, I've just approved all CI jobs to run against this PR.

marco-langer commented 2 years ago

The PR is ready for review, if we are currently on C++11.

However, yesterday I saw a comment of you saying that we are actually able to develop in C++14 already. If this is the case (the GIL github project description is not correct then), I would like to refactor this PR again to replace the functor and use a generic lambda to follow the suggestion in the linked issue.

mloskot commented 2 years ago

In commit https://github.com/boostorg/gil/commit/8b1c2d3ea4eb5bf0a1e7e093862962313dd6c2bb we announced possibility to switch over to C++14 in the RELEASES.md. We also have started with @sdebionne and @lpranam discussion about switch to C++17, see https://github.com/boostorg/gil/pull/526#issuecomment-715220681 and https://github.com/boostorg/gil/pull/526#issuecomment-718007963 and there probably are places too.

There are lots of details to consider, but if you'd like to discuss pros and cons of the C++ upgrade in the context of GIL enhancements, please feel free and welcomed to open discussion.

marco-langer commented 2 years ago

You are right, discussing further steps for a possible C++14/C++17 version upgrade sounds like a good idea.

Let's keep this PR with C++11-style functors then and postpone a further modernization.