dbus2 / zbus

Rust D-Bus crate.
Other
316 stars 70 forks source link

🐛 implement `Hash` and `Eq` property for Value::F64 #872

Closed m4rch3n1ng closed 1 week ago

m4rch3n1ng commented 1 week ago

the current implementation of Hash for zvariant::Value doesn't hold the property described in the rust rust standard library in Hash and Eq:

use std::hash::{DefaultHasher, Hash, Hasher};

fn main() {
    let v1 = zvariant::Value::F64(0.);
    let v2 = zvariant::Value::F64(-0.);
    println!("{} == {} : {}", v1, v2, v1 == v2);

    let mut hasher = DefaultHasher::new();
    v1.hash(&mut hasher);
    let h1 = hasher.finish();

    let mut hasher = DefaultHasher::new();
    v2.hash(&mut hasher);
    let h2 = hasher.finish();

    println!("{:#x} == {:#x} : {}", h1, h2, h1 == h2);
}
0. == -0. : true
0x3885e79ab4b1bb3a == 0xe3da94884cfc9c4 : false

implementing Hash for an f64 comes with a host of annoying issues, like being able to insert multiple Variant::F64(f64::NAN) as keys into the map and not being able to retrieve them anymore, but it doesn't break this specific property, as f64::NAN never evaluates to true with an == operation, so it still (technically) holds the k1 == k2 -> hash(k1) == hash(k2) property.

this fixes the Hash and Eq property by hashing 0. and -0. the same way.

0. == -0. : true
0x3885e79ab4b1bb3a == 0x3885e79ab4b1bb3a : true
m4rch3n1ng commented 1 week ago

on a side note, is there a reason to use f64::to_le_bytes() over simply using f64::to_bits()? the latter should be quicker (note: i haven't done any benchmarks), since the former calls f64::to_bits().

zeenix commented 1 week ago

on a side note, is there a reason to use f64::to_le_bytes() over simply using f64::to_bits()? the latter should be quicker (note: i haven't done any benchmarks), since the former calls f64::to_bits().

It's just a wrapper then and will be optimised out by the compiler anyway with any level of optimisation enabled. Even if not, the cost would be insignificant. I wouldn't worry about it. :)