Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Added for_each_pixel overload for any_image #648

Merged
merged 3 commits into from
Apr 30, 2022

Conversation

marco-langer
Copy link
Contributor

@marco-langer marco-langer commented Apr 24, 2022

Description

Fixes #579

References

Tasklist

  • Add test case(s)
  • Ensure all CI builds pass
  • Review and approve

@marco-langer marco-langer changed the title added for_each_pixel overload for any_image #579 added for_each_pixel overload for any_image Apr 24, 2022
@marco-langer marco-langer changed the title added for_each_pixel overload for any_image [WIP] added for_each_pixel overload for any_image Apr 24, 2022
@marco-langer
Copy link
Contributor Author

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
Copy link

codecov bot commented Apr 25, 2022

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 mloskot marked this pull request as draft April 25, 2022 15:21
@mloskot mloskot added the cat/enhancement Improvements, but not fixes addressing identified bugs label Apr 25, 2022
@mloskot
Copy link
Member

mloskot commented Apr 25, 2022

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

@marco-langer marco-langer changed the title [WIP] added for_each_pixel overload for any_image added for_each_pixel overload for any_image Apr 26, 2022
@marco-langer marco-langer marked this pull request as ready for review April 26, 2022 16:43
@mloskot mloskot changed the title added for_each_pixel overload for any_image feat: Added for_each_pixel overload for any_image Apr 29, 2022
@mloskot
Copy link
Member

mloskot commented Apr 29, 2022

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

@marco-langer
Copy link
Contributor Author

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
Copy link
Member

mloskot commented Apr 30, 2022

In commit 8b1c2d3 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 #526 (comment) and #526 (comment) 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
Copy link
Contributor Author

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.

Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you!

@mloskot mloskot merged commit c063d1c into boostorg:develop Apr 30, 2022
@mloskot mloskot added the ext/dynamic_image boost/gil/extension/dynamic_image label Apr 30, 2022
@mloskot mloskot added this to the Boost 1.78+ milestone Apr 30, 2022
@mloskot mloskot mentioned this pull request May 12, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat/enhancement Improvements, but not fixes addressing identified bugs ext/dynamic_image boost/gil/extension/dynamic_image
Projects
None yet
Development

Successfully merging this pull request may close these issues.

for_each_pixel not work for any_image
2 participants