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 support for movability to random generators. Fix POSIX random provider #74

Merged
merged 1 commit into from Jun 18, 2018

Conversation

Projects
None yet
3 participants
@Lastique
Member

Lastique commented Jun 17, 2018

The commit adds support for move constructors and assignment to random UUID
generators and random providers. Also, in POSIX random provider, the commit
improves handling of return value from ::read(), which returns -1
in case of error and may return 0 in case of success. Retry the call when
it was interrupted before reading any bytes from /dev/urandom.

Added new tests for movability.

This fixes #71

@Lastique Lastique force-pushed the Lastique:random_gen_movability branch from fc70b82 to 479cd12 Jun 17, 2018

@Lastique Lastique changed the title from Add support for movamility to random generators. Fix POSIX random provider to Add support for movability to random generators. Fix POSIX random provider Jun 17, 2018

@jeking3

This comment has been minimized.

Collaborator

jeking3 commented Jun 17, 2018

Interesting that you chose to do this without boost::move, any particular reason?

@jeking3

This comment has been minimized.

Collaborator

jeking3 commented Jun 17, 2018

Ideally we should have a compile-fail test that attempts copy by constructor and another copy by assignment to make sure those are properly disabled. This would ensure in the future nobody re-enables it by accident.

@Lastique

This comment has been minimized.

Member

Lastique commented Jun 17, 2018

Interesting that you chose to do this without boost::move, any particular reason?

I did not want to pull in Boost.Move as a dependency. But if you think it's worth supporting move operations in C++03 then it can be added.

@jeking3

This comment has been minimized.

Collaborator

jeking3 commented Jun 17, 2018

I was trying to do it with Boost.Move but it wasn't cooperating with me. It was probably me though. :) I'd prefer it to be implementation that works with both language levels. The dependencies in move are pretty light:

https://pdimov.github.io/boostdep-report/develop/move.html

I also offered to help the maintainer of Boost.Move add CI to that repository when I found it had none.

Add support for movability to random generators. Fix POSIX random pro…
…vider.

The commit adds support for move constructors and assignment to random UUID
generators and random providers. Also, in POSIX random provider, the commit
improves handling of return value from ::read(), which returns -1
in case of error and may return 0 in case of success. Retry the call when
it was interrupted before reading any bytes from /dev/urandom.

Added new tests for movability and compile-fail tests for no copyability.

This addresses GitHub issue:

#71

@Lastique Lastique force-pushed the Lastique:random_gen_movability branch from 479cd12 to 429daf0 Jun 17, 2018

@codecov

This comment has been minimized.

codecov bot commented Jun 17, 2018

Codecov Report

Merging #74 into develop will increase coverage by 1.43%.
The diff coverage is 77.19%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #74      +/-   ##
===========================================
+ Coverage    78.34%   79.78%   +1.43%     
===========================================
  Files           13       13              
  Lines          605      638      +33     
  Branches       156      154       -2     
===========================================
+ Hits           474      509      +35     
  Misses          17       17              
+ Partials       114      112       -2
Impacted Files Coverage Δ
include/boost/uuid/random_generator.hpp 72.91% <68.96%> (+15.34%) ⬆️
...nclude/boost/uuid/detail/random_provider_posix.ipp 86.48% <85.71%> (+10.48%) ⬆️
include/boost/uuid/detail/random_provider.hpp 92.3% <85.71%> (+6.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d0aa87...429daf0. Read the comment docs.

@Lastique

This comment has been minimized.

Member

Lastique commented Jun 17, 2018

I've added support for C++03 and compile-fail tests.

@MarcelRaad

Works fine for me with VS 2017 Update 7.3 and Update 8 Preview 2.0.

@jeking3

This comment has been minimized.

Collaborator

jeking3 commented Jun 18, 2018

Would be nice to add a unit test for the EINTR handling at some point. Thanks for the help on move.

@jeking3 jeking3 merged commit 4a9f620 into boostorg:develop Jun 18, 2018

3 of 4 checks passed

codecov/patch 77.19% of diff hit (target 78.34%)
Details
codecov/project 79.78% (+1.43%) compared to 0d0aa87
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Lastique Lastique deleted the Lastique:random_gen_movability branch Jun 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment