editoast: use type SupportedSignalingSystem everywhere#16982
editoast: use type SupportedSignalingSystem everywhere#16982woshilapin wants to merge 2 commits into
SupportedSignalingSystem everywhere#16982Conversation
woshilapin
force-pushed
the
wsl/editoast-use-type-SupportedSignalingSystem-everywhere
branch
2 times, most recently
from
c2c82f0 to
4eac28f
Compare
| .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 |
There was a problem hiding this comment.
we really pulling a lib for 2 lines
| .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, | |
| }) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 🤷
woshilapin
force-pushed
the
wsl/editoast-use-type-SupportedSignalingSystem-everywhere
branch
from
20656d0 to
dc89962
Compare
github-actions
Bot
added
area:front
|
Also, on all the pathfinding API, we don’t need a full-fledge `SupportedSignalingSystem`, but only the variant is enough (for example, we don’t need the braking curves of ETCS). So we derive a `SignalingSystemVariant` for those API, with the help of `strum`. It will be used in a future commit. Also provides hashing function that will be useful for hashing `HashSet<SupportedSignalingSystem>` (and the corresponding `*Variant`). We create a bitmask of all the present variants, using the discriminant of the variant’s enum. This should be unique, even if the order of the iteration is different. The function that hashes a `HashSet<SupportedSignalingSystemVariant>` is not trivial, and is supposed to respect a few properties. `proptest` is a library that basically do controlled fuzzing, which can help assert those properties. Signed-off-by: Jean SIMARD <woshilapin@tuziwo.info>
In some places, we were converting to `String` with no real added value. Let’s keep `HashSet<SupportedSignalingSystem>` everywhere, avoiding conversions. One of the inconvenient is the implementation of `Hash` for some of the `struct` that contains it (`HashSet` doesn’t implement `Hash`). With the help of `educe`, we can plug a custom function to hash such a `HashSet`. Signed-off-by: Jean SIMARD <woshilapin@tuziwo.info>
woshilapin
force-pushed
the
wsl/editoast-use-type-SupportedSignalingSystem-everywhere
branch
from
dc89962 to
204cb54
Compare
hhirtz
left a comment
hhirtz
left a comment
There was a problem hiding this comment.
not sure adding more macro is a good step towards lower compilation times though... 🤔
| #[strum_discriminants(strum(to_string = "ETCS_LEVEL2"))] | ||
| #[strum_discriminants(serde(rename = "ETCS_LEVEL2"))] |
There was a problem hiding this comment.
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.
| .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 |
There was a problem hiding this comment.
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
| 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); |
There was a problem hiding this comment.
| 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... :/
3 participants
In some places, we were converting to
Stringwith no real added value. Let’s keepHashSet<SupportedSignalingSystem>everywhere, avoiding conversions.One of the inconvenient is the implementation of
Hashfor some of thestructthat contains it (HashSetdoesn’t implementHash). With the help ofeduce, we can plug a custom function to hash such aHashSet. And we consider create a bitmask of all the present variant, using the discriminant of the variant’s enum. This should be unique, even if the order of the iteration is different.