Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 79 additions & 2 deletions editoast/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions editoast/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ postgis_diesel = { version = "3.1.0", features = ["serde"] }
postgres-openssl = "0.5.3"
pretty_assertions = "1.4.1"
proc-macro2 = "1.0"
proptest = "1.11.0"
quote = "1.0"
rand = "0.10.1"
rangemap = "1.7.1"
Expand Down
1 change: 1 addition & 0 deletions editoast/schemas/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ utoipa.workspace = true
uuid.workspace = true

[dev-dependencies]
proptest.workspace = true
Comment thread
leovalais marked this conversation as resolved.
rstest.workspace = true

[lints]
Expand Down
166 changes: 165 additions & 1 deletion editoast/schemas/src/rolling_stock/supported_signaling_system.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
use std::collections::HashSet;
use std::hash::Hash;
use std::hash::Hasher;

use educe::Educe;
use serde::Deserialize;
use serde::Serialize;
Expand All @@ -6,20 +10,180 @@ use utoipa::ToSchema;

use crate::rolling_stock::EtcsBrakeParams;

#[derive(Clone, Debug, Deserialize, Serialize, Display, Educe, ToSchema, strum::IntoStaticStr)]
#[derive(
Clone,
Debug,
Deserialize,
Serialize,
Display,
Educe,
ToSchema,
strum::IntoStaticStr,
strum::EnumDiscriminants,
)]
#[educe(Hash, Eq, PartialEq)]
#[serde(tag = "type")]
#[schema(title_variants)]
#[allow(clippy::large_enum_variant)]
#[strum_discriminants(
name(SupportedSignalingSystemVariant),
derive(Deserialize, Serialize, Display, Hash, ToSchema, strum::IntoStaticStr)
)]
pub enum SupportedSignalingSystem {
BAL,
BAPR,
TVM300,
TVM430,
#[strum(to_string = "ETCS_LEVEL2")]
#[serde(rename = "ETCS_LEVEL2")]
#[strum_discriminants(strum(to_string = "ETCS_LEVEL2"))]
#[strum_discriminants(serde(rename = "ETCS_LEVEL2"))]
Comment on lines +39 to +40
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just a comment but im not a fan of these kind of attribute macros, since it slows down compilation and linting and doesn't really make the code easier to read than plainly writing the into::<str>() functions.

EtcsLevel2 {
#[educe(Hash(ignore), PartialEq(ignore))]
brake_params: EtcsBrakeParams,
},
}

impl SupportedSignalingSystem {
const fn discriminant(&self) -> u32 {
match self {
SupportedSignalingSystem::BAL => 0,
SupportedSignalingSystem::BAPR => 1,
SupportedSignalingSystem::TVM300 => 2,
SupportedSignalingSystem::TVM430 => 3,
SupportedSignalingSystem::EtcsLevel2 { .. } => 4,
}
}
}

impl SupportedSignalingSystemVariant {
const fn discriminant(&self) -> u32 {
match self {
SupportedSignalingSystemVariant::BAL => 0,
SupportedSignalingSystemVariant::BAPR => 1,
SupportedSignalingSystemVariant::TVM300 => 2,
SupportedSignalingSystemVariant::TVM430 => 3,
SupportedSignalingSystemVariant::EtcsLevel2 => 4,
}
}
}

pub fn hashing_supported_signaling_systems<H>(
iter: &HashSet<SupportedSignalingSystem>,
state: &mut H,
) where
H: Hasher,
{
let hash: u64 = iter
.iter()
.map(SupportedSignalingSystem::discriminant)
// Each discriminant correspond to one specific bit of a `u64`
.map(|discriminant| 1u64.wrapping_shl(discriminant))
// Create a unique bitmask
.reduce(std::ops::BitOr::bitor)
.unwrap_or(0);
Hash::hash(&hash, state)
}

pub fn hashing_supported_signaling_systems_variant<H>(
iter: &HashSet<SupportedSignalingSystemVariant>,
state: &mut H,
) where
H: Hasher,
{
let hash: u64 = iter
.iter()
.map(SupportedSignalingSystemVariant::discriminant)
// Each discriminant correspond to one specific bit of a `u64`
.map(|discriminant| 1u64.wrapping_shl(discriminant))
// Create a unique bitmask
Comment on lines +79 to +99
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we really pulling a lib for 2 lines

Suggested change
.map(SupportedSignalingSystem::discriminant)
.map(u32::from)
// Each discriminant correspond to one specific bit of a `u64`
.map(|discriminant| 1u64.wrapping_shl(discriminant))
// Create a unique bitmask
.map(|system| match system {
SupportedSignalingSystem::BAL => 1 << 0,
SupportedSignalingSystem::BAPR => 1 << 1,
SupportedSignalingSystem::TVM300 => 1 << 2,
SupportedSignalingSystem::TVM430 => 1 << 3,
SupportedSignalingSystem::EtcsLevel2 { .. } => 1 << 4,
})

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why not use the match expr directly like above? the discriminant only seems like an implementation detail of the hashing functions, not some property we want on supported signaling systems.

also on the variant enum you could add #[repr(u32)] then implement discriminant with self as u32

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't mind using strum for this, its already in our dep tree and used throughout the codebase. I'm not too worried about compilation times of the schema crate: it sits on top of all others and compiles almost instantly (and in parallel of other crates).

I'm fine with matching directly as well though 🤷

.reduce(std::ops::BitOr::bitor)
.unwrap_or(0);
Hash::hash(&hash, state)
}

#[cfg(test)]
mod tests {
use super::*;

mod hash {
use super::*;
use SupportedSignalingSystemVariant::*;

use proptest::prelude::*;

fn supported_signaling_system_strategy_variant()
-> impl Strategy<Value = SupportedSignalingSystemVariant> {
prop_oneof![
Just(BAL),
Just(BAPR),
Just(TVM300),
Just(TVM430),
Just(EtcsLevel2)
]
.boxed()
}

fn hash(iter: &HashSet<SupportedSignalingSystemVariant>) -> u64 {
let mut state = std::hash::DefaultHasher::new();
hashing_supported_signaling_systems_variant(iter, &mut state);
state.finish()
}

proptest! {
#[test]
fn remove_element_has_different_hash(
mut a in proptest::collection::hash_set(supported_signaling_system_strategy_variant(), 0..10),
s in supported_signaling_system_strategy_variant(),
) {
let hash_before = hash(&a);
let is_removed = a.remove(&s);
let hash_after = hash(&a);
if is_removed {
assert_ne!(hash_before, hash_after);
} else {
assert_eq!(hash_before, hash_after);
}
}

#[test]
fn same_hashset_has_same_hash(
a in proptest::collection::hash_set(supported_signaling_system_strategy_variant(), 0..10),
b in proptest::collection::hash_set(supported_signaling_system_strategy_variant(), 0..10)
) {
if a == b {
assert_eq!(hash(&a), hash(&b));
} else {
assert_ne!(hash(&a), hash(&b));
}
}

#[test]
fn same_hash_has_an_empty_difference(
a in proptest::collection::hash_set(supported_signaling_system_strategy_variant(), 0..10),
b in proptest::collection::hash_set(supported_signaling_system_strategy_variant(), 0..10)
) {
if hash(&a) == hash(&b) {
assert!(a.symmetric_difference(&b).count() == 0);
} else {
assert!(a.symmetric_difference(&b).count() > 0);
}
}

#[test]
fn different_signaling_system_has_different_hash(
a in supported_signaling_system_strategy_variant(),
b in supported_signaling_system_strategy_variant(),
) {
let same_signaling_system= a == b;
let hash_a = hash(&HashSet::from([a]));
let hash_b = hash(&HashSet::from([b]));
if same_signaling_system {
assert_eq!(hash_a, hash_b);
} else {
assert_ne!(hash_a , hash_b);
Comment on lines +178 to +184
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
let same_signaling_system= a == b;
let hash_a = hash(&HashSet::from([a]));
let hash_b = hash(&HashSet::from([b]));
if same_signaling_system {
assert_eq!(hash_a, hash_b);
} else {
assert_ne!(hash_a , hash_b);
let same_signaling_system = a == b;
let hash_a = hash(&HashSet::from([a]));
let hash_b = hash(&HashSet::from([b]));
if same_signaling_system {
assert_eq!(hash_a, hash_b);
} else {
assert_ne!(hash_a, hash_b);

I guess rustfmt doesn't work well here... :/

}
}
}
}
}