-
Notifications
You must be signed in to change notification settings - Fork 421
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
Derive PartialEq+Eq. #975
Derive PartialEq+Eq. #975
Changes from 7 commits
264c668
3ec2677
1ac7a99
1f985df
617355d
20459bf
b20a21e
52c94bb
85e2825
17afa0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
[package] | ||
name = "rand" | ||
version = "0.7.3" | ||
version = "0.7.4" | ||
authors = ["The Rand Project Developers", "The Rust Project Developers"] | ||
license = "MIT OR Apache-2.0" | ||
readme = "README.md" | ||
|
@@ -55,7 +55,7 @@ members = [ | |
] | ||
|
||
[dependencies] | ||
rand_core = { path = "rand_core", version = "0.5.1" } | ||
rand_core = { path = "rand_core", version = "0.5.2" } | ||
rand_pcg = { path = "rand_pcg", version = "0.2", optional = true } | ||
log = { version = "0.4.4", optional = true } | ||
|
||
|
@@ -73,7 +73,7 @@ libc = { version = "0.2.22", optional = true, default-features = false } | |
# Emscripten does not support 128-bit integers, which are used by ChaCha code. | ||
# We work around this by using a different RNG. | ||
[target.'cfg(not(target_os = "emscripten"))'.dependencies] | ||
rand_chacha = { path = "rand_chacha", version = "0.2.1", default-features = false, optional = true } | ||
rand_chacha = { path = "rand_chacha", version = "0.2.2", default-features = false, optional = true } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your change will be in 0.2.3, if that's why you updated the number. (If not, well, 0.2.2 was not a mandatory upgrade.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, good catch, will fix. |
||
[target.'cfg(target_os = "emscripten")'.dependencies] | ||
rand_hc = { path = "rand_hc", version = "0.2", optional = true } | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,11 +61,21 @@ impl<T> fmt::Debug for Array64<T> { | |
write!(f, "Array64 {{}}") | ||
} | ||
} | ||
impl<T> ::core::cmp::PartialEq for Array64<T> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some places used full paths for the ops traits, so I wasn't sure if that's preferred here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't care a lot. Since Edition 2018 you don't need the leading There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you have no preference, I'll just leave as is. If you'd prefer I remove the leading |
||
where T: ::core::cmp::PartialEq { | ||
fn eq(&self, other: &Array64<T>) -> bool { | ||
&self.0[..] == &other.0[..] | ||
} | ||
} | ||
impl<T> ::core::cmp::Eq for Array64<T> | ||
where T: ::core::cmp::Eq { | ||
} | ||
|
||
|
||
macro_rules! chacha_impl { | ||
($ChaChaXCore:ident, $ChaChaXRng:ident, $rounds:expr, $doc:expr) => { | ||
#[doc=$doc] | ||
#[derive(Clone)] | ||
#[derive(Clone, PartialEq, Eq)] | ||
pub struct $ChaChaXCore { | ||
state: ChaCha, | ||
} | ||
|
@@ -140,7 +150,7 @@ macro_rules! chacha_impl { | |
/// | ||
/// [^2]: [eSTREAM: the ECRYPT Stream Cipher Project]( | ||
/// http://www.ecrypt.eu.org/stream/) | ||
#[derive(Clone, Debug)] | ||
#[derive(Clone, Debug, PartialEq, Eq)] | ||
pub struct $ChaChaXRng { | ||
rng: BlockRng<$ChaChaXCore>, | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,24 @@ pub struct ChaCha { | |
pub(crate) d: vec128_storage, | ||
} | ||
|
||
// Custom PartialEq implementation as `vec128_storage` doesn't implement it. | ||
impl ::core::cmp::PartialEq for ChaCha { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems pretty non-ideal, but I'm hoping to not have to also try to get this change into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing wrong with a custom implementation, but that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should I go try to get There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could ask @kazcw about that, but looks to me like you can just do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My previous version used There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see this code was added since I don't wish to add this unsafe code, which makes us blocked on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dhardy Sure, I can do that this weekend. |
||
fn eq(&self, other: &ChaCha) -> bool { | ||
unsafe { | ||
::core::mem::transmute::<vec128_storage, [u32; 4]>(self.b) | ||
== ::core::mem::transmute::<vec128_storage, [u32; 4]>(other.b) | ||
&& ::core::mem::transmute::<vec128_storage, [u32; 4]>(self.c) | ||
== ::core::mem::transmute::<vec128_storage, [u32; 4]>(other.c) | ||
&& ::core::mem::transmute::<vec128_storage, [u32; 4]>(self.d) | ||
== ::core::mem::transmute::<vec128_storage, [u32; 4]>(other.d) | ||
} | ||
} | ||
} | ||
|
||
// Custom Eq implementation as `vec128_storage` doesn't implement it. | ||
impl ::core::cmp::Eq for ChaCha { | ||
} | ||
|
||
#[derive(Clone)] | ||
pub struct State<V> { | ||
pub(crate) a: V, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,7 +67,7 @@ pub trait BlockRngCore { | |
|
||
/// Results type. This is the 'block' an RNG implementing `BlockRngCore` | ||
/// generates, which will usually be an array like `[u32; 16]`. | ||
type Results: AsRef<[Self::Item]> + AsMut<[Self::Item]> + Default; | ||
type Results: AsRef<[Self::Item]> + AsMut<[Self::Item]> + Default + PartialEq + Eq; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might be okay, but need to think about it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively I can conditionally do |
||
|
||
/// Generate a new block of results. | ||
fn generate(&mut self, results: &mut Self::Results); | ||
|
@@ -109,7 +109,7 @@ pub trait BlockRngCore { | |
/// [`next_u64`]: RngCore::next_u64 | ||
/// [`fill_bytes`]: RngCore::fill_bytes | ||
/// [`try_fill_bytes`]: RngCore::try_fill_bytes | ||
#[derive(Clone)] | ||
#[derive(Clone, PartialEq, Eq)] | ||
#[cfg_attr(feature = "serde1", derive(Serialize, Deserialize))] | ||
pub struct BlockRng<R: BlockRngCore + ?Sized> { | ||
results: R::Results, | ||
|
@@ -272,7 +272,7 @@ impl<R: BlockRngCore + SeedableRng> SeedableRng for BlockRng<R> { | |
/// [`next_u64`]: RngCore::next_u64 | ||
/// [`fill_bytes`]: RngCore::fill_bytes | ||
/// [`try_fill_bytes`]: RngCore::try_fill_bytes | ||
#[derive(Clone)] | ||
#[derive(Clone, PartialEq, Eq)] | ||
#[cfg_attr(feature = "serde1", derive(Serialize, Deserialize))] | ||
pub struct BlockRng64<R: BlockRngCore + ?Sized> { | ||
results: R::Results, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,7 +75,7 @@ use rand_core::{CryptoRng, Error, RngCore, SeedableRng}; | |
/// [`BlockRngCore`]: rand_core::block::BlockRngCore | ||
/// [`ReseedingRng::new`]: ReseedingRng::new | ||
/// [`reseed()`]: ReseedingRng::reseed | ||
#[derive(Debug)] | ||
#[derive(Debug, PartialEq, Eq)] | ||
pub struct ReseedingRng<R, Rsdr>(BlockRng<ReseedingCore<R, Rsdr>>) | ||
where | ||
R: BlockRngCore + SeedableRng, | ||
|
@@ -147,7 +147,7 @@ where | |
{ | ||
} | ||
|
||
#[derive(Debug)] | ||
#[derive(Debug, PartialEq, Eq)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this makes sense. We have a reseed-on-fork feature which in a sense violates We also have no use for this, since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll get rid of this then. From the description, this sounded like a wrapper class that would be comparable if the RNG it's wrapping is comparable, and the example uses a ChaCha20Core, though I'm sure there is some nuance I'm missing 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I should get rid of this on both |
||
struct ReseedingCore<R, Rsdr> { | ||
inner: R, | ||
reseeder: Rsdr, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't make any sense for
ThreadRng
because that's just a handle. Handles cannot cross thread boundaries so any two handles which can be compared should be equal. I'm also not sure it's useful on the reseeding constructs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. I was thinking that comparing the internal pointers would be reasonable (assuming that's what the derive does for
NonNull
). You bring up a good point about it not being something you'd store and pass across threads. I'll remove the derive on that one.