That3Percent / tree-buf

An experimental serialization system written in Rust
MIT License
256 stars 8 forks source link

remove strict float comparisons #28

Open danieleades opened 3 years ago

danieleades commented 3 years ago

this library uses a bunch of strict float comparisons.

would you consider a pull request to implement slightly safer float comparisons? possibly using float_cmp or simply making use of the num_traits::float::Float::epsilon method (either way will probably require a where T: num_traits::Float: trait bound)

unless there are sound reasons we can assume they are identically equal in these comparisons?

if so, would you consider a pull request to silence this lint? Strict float comparisons are denied by Clippy, and will prevent using Clippy in any CI pipeline.

That3Percent commented 3 years ago

I would accept a PR to silence this lint, but not to remove strict equality comparisons. Thanks for asking!

Since this is a serialization library, one of the key tenets is that if you round trip data through it, the binary representation of that data should be unchanged. We don't want to be making assumptions for our users about what is a tolerable loss of data because we do not know their use-case a-priori.

So, while float_cmp has it's place, I don't believe that place is here. It's more for doing math like checking if a point is inside a triangle and similar.

There is an exception to this binary equivalence rule when lossy encodings are enabled - but that is opt-in, has a configurable tolerance, and IIRC is not the part of the code where these equality checks are occurring. I might accept a PR that used float_cmp when that feature is enabled The lint silencing would still be required since there would now just be multiple code paths.

danieleades commented 3 years ago

ah thanks for the explanation. I had a feeling it was going to be something along those lines. I'll submit a pull request to supress the lint and call it a day.

I will also take a look at the lossy encodings and see if it's worth touching the float comparisons there