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

Add move semantics to image class #457

Merged
merged 6 commits into from
Apr 6, 2020

Conversation

sdebionne
Copy link
Contributor

@sdebionne sdebionne commented Mar 19, 2020

Description

Add move semantic to image.

References

Tasklist

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

include/boost/gil/image.hpp Outdated Show resolved Hide resolved
@sdebionne sdebionne force-pushed the image-move-sematic branch 2 times, most recently from 716a867 to 580fbf7 Compare March 19, 2020 14:57
@mloskot mloskot changed the title Image move semantic Add move semantics to image class Mar 19, 2020
@sdebionne sdebionne force-pushed the image-move-sematic branch from 580fbf7 to c965b30 Compare March 26, 2020 15:18
@mloskot mloskot added the cat/enhancement Improvements, but not fixes addressing identified bugs label Mar 26, 2020
mloskot added a commit to mloskot/gil that referenced this pull request Mar 26, 2020
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.

@sdebionne Thanks for the contribution. Here is my first review.

test/core/image/image.cpp Outdated Show resolved Hide resolved
include/boost/gil/image.hpp Outdated Show resolved Hide resolved
include/boost/gil/image.hpp Show resolved Hide resolved
include/boost/gil/image.hpp Show resolved Hide resolved
test/core/image/image.cpp Outdated Show resolved Hide resolved
include/boost/gil/image.hpp Outdated Show resolved Hide resolved
@sdebionne
Copy link
Contributor Author

@mloskot I rebased on develop and made the changes we discussed. If the tests pass, I'll mark it ready for review.

@mloskot
Copy link
Member

mloskot commented Mar 30, 2020

@sdebionne I also asked for feedback on the Boost list in [gil] Review of move assignment operator for image class

@mloskot
Copy link
Member

mloskot commented Apr 1, 2020

Found a good comment related to solutions we are discussing here:

If your allocator type is stateless and/or sets is_always_equal,
then the settings of POCCA/POCMA/POCS don’t really matter
and might just as well be inconsistent.
For historical reasons, std::allocator falls into that category.

from https://quuxplusone.github.io/blog/2019/08/02/the-tough-guide-to-cpp-acronyms/#pocca-pocma-pocs

@sdebionne
Copy link
Contributor Author

It explains why MSVC's STL has this special case for std::allocator_traits::is_always_equal and treat it as a pocma. Since we don't have it in C++11, is it OK to approximate with std::is_empty?

I'll give it a go by the end of the week.

@mloskot
Copy link
Member

mloskot commented Apr 2, 2020

is it OK to approximate with std::is_empty?

I think, it is and http://eel.is/c++draft/allocator.traits#types-10 says

10 Type: Alloc​::​is_­always_­equal if the qualified-id Alloc​::​is_­always_­equal is valid and
denotes a type ([temp.deduct]); otherwise is_­empty<Alloc>​::​type.

Additionally, MSVC++ std lib has this base case template

template <class _Ty, class = void>
struct _Get_is_always_equal {
    using type = typename is_empty<_Ty>::type;
};

@sdebionne sdebionne force-pushed the image-move-sematic branch from 6a79741 to f9e3684 Compare April 5, 2020 08:36
@sdebionne
Copy link
Contributor Author

@mloskot Are you aware that "the hosted agent vs2015-win2012r2 was permanently removed on April 2nd, 2020. See https://aka.ms/blocked-hosted-agent for more information" (from azure pipelines)?

@mloskot
Copy link
Member

mloskot commented Apr 5, 2020

@sdebionne

Are you aware that "the hosted agent vs2015-win2012r2 was permanently removed on April 2nd, 2020

Yes, I am. @sdebionne I have removed the VS2015 job now 5d531ed

BTW, I am going to use this as opportunity to switch to https://github.com/boostorg/boost-ci/ setup, but have been waiting for merge of some major refactoring of boost-ci. I will try to switch GIL AzP setup to boost-ci today/tomorrow.

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.

@sdebionne Looks good to me and ready to merge. Thanks!

Do you want to update this PR to get rid of the VS2015 job?

@sdebionne sdebionne force-pushed the image-move-sematic branch from f9e3684 to a9ee91e Compare April 6, 2020 07:50
@sdebionne sdebionne marked this pull request as ready for review April 6, 2020 07:51
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

Thanks for the contribution!

@mloskot mloskot merged commit 9b055f2 into boostorg:develop Apr 6, 2020
@mloskot mloskot added this to the Boost 1.73 milestone Apr 6, 2020
mloskot added a commit that referenced this pull request Apr 6, 2020
@sdebionne sdebionne deleted the image-move-sematic branch April 7, 2020 08:55
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants