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

Update rand_core::Error in line with getrandom::Error #864

Merged
merged 4 commits into from
Aug 28, 2019

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Aug 9, 2019

This mirrors the changes in rust-random/getrandom#54 . All changes to rand_core are additive (not breaking).

rand_jitter gets updated to put its error codes within the appropriate range, and also a minor "fix": use of repr(u32) on the enum (which I don't believe is a breaking change, since either way for e: TimerError, e as u32 and e as isize are supported, while u32::from(e) is not).

Open question: should we copy rand_jitter error descriptions into rand_core when cfg(all(feature="getrandom", not(feature="std")))? I'm leaning against doing this, since it increases crate size while benefiting very few people.

Also see #837.

@vks
Copy link
Collaborator

vks commented Aug 9, 2019

So changing the error codes is not considered a breaking change for rand_jitter?

I'm fine with not copying the error descriptions, since I consider rand_jitter to be kind of deprecated.

#[cfg(all(feature="getrandom", not(feature="std")))] {
getrandom::Error::from(self.code).fmt(f)
}
#[cfg(not(feature="getrandom"))] {
Copy link
Member

Choose a reason for hiding this comment

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

You should use cfg(not(any(feature="getrandom", feature="std"))). Same for Display impl. Maybe use cfg_if to be less error-prone?

Copy link
Member Author

Choose a reason for hiding this comment

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

std implies getrandom

fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
self.inner.source()
}
}

#[cfg(feature="std")]
impl From<Error> for std::io::Error {
#[inline]
fn from(error: Error) -> Self {
std::io::Error::new(std::io::ErrorKind::Other, error)
Copy link
Member

@newpavlov newpavlov Aug 9, 2019

Choose a reason for hiding this comment

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

Maybe write something like this instead?

if let Some(code) = error.raw_os_error() {
    std::io::Error::from_raw_os_error(code)
} else {
    std::io::Error::new(std::io::ErrorKind::Other, error)
}

@@ -10,24 +10,28 @@
use rand_core::Error;
use core::fmt;

/// Base code for all `JitterRng` errors
const ERROR_BASE: u32 = 0xAE53_0400;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe write it like const ERROR_BASE: u32 = Error::INTERNAL_START + <some const>?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this actually clearer? To me it's not.

@dhardy
Copy link
Member Author

dhardy commented Aug 22, 2019

@newpavlov I added your suggestion; sorry about the delay. I think the other points you mentioned can be left?

@newpavlov
Copy link
Member

@dhardy Yes, they were just suggestions, so you can merge this PR as-is.

@dhardy
Copy link
Member Author

dhardy commented Aug 28, 2019

Failures: cache timeout and Redox failure. Merging.

@dhardy dhardy merged commit d877ed5 into rust-random:master Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants