Janitor: Fix clippy error about f32 comparison #289
Conversation
|
This is actually a clippy error, so it stops processing of the crate and potentially hides other clippy errors and warnings. |
| @@ -235,9 +235,9 @@ impl From<RgbaColor<f32>> for HsvaColor { | |||
| let hue = 60. | |||
| * if chroma == 0. { | |||
| 0.0 | |||
| } else if max == red { | |||
| } else if (max - red).abs() < f32::EPSILON { | |||
There was a problem hiding this comment.
I think in this case we might be better off with #[allow(clippy::float_cmp)]. rust-lang/rust-clippy#6816 suggests that this is basically the same and in this case I can’t think of a better epsilon either.
No strong opinion though. @ogoffart has more experience on this matter I believe
There was a problem hiding this comment.
Hmmm... How about defining our own epsilon of 0.00001 or something? That's smaller than 1/(2^16 - 1). That should be good enough for color spaces up to 16 bit per channel.
I have seen 10bit/color channel before, but 16bit should be safe enough for the time being.
There was a problem hiding this comment.
Maybe it could be
| } else if (max - red).abs() < f32::EPSILON { | |
| } else if max <= red { |
Since we know that max is the maximum between red, green, and blue.
There was a problem hiding this comment.
You're right, we know that max must be one of the three and it's not the result of another arithmetic operation that might affect precision. I totally missed that :)
<= sounds like it would also silence clippy, indeed.
I favour an explicit #[allow(clippy::float_cmp)] with a comment that says that it's okay to compare because it can only be one of the three options.
A fourth way would be to have let red_is_max: bool = ...; let green_is_max: bool = ...; ... ;-)
What this also means is that Tobias' variant is also correct, despite the rust-lang issue.
hunger
force-pushed
the
pr/cargo_error
branch
2 times, most recently
from
aee5230 to
d8859cc
Compare
|
This is probably over-doing it a bit, considering what Olivier said: max can only be one of three values in this case. |
|
How about just allowing this here (with a comment stating why this is OK)? |
| @@ -232,6 +232,7 @@ impl From<RgbaColor<f32>> for HsvaColor { | |||
| let max = red.max(green).max(blue); | |||
| let chroma = max - min; | |||
|
|
|||
| #[allow(clippy::float_cmp)] // `max` is either `red`, `green` or `blue` | |||
This is a code change, but it should not change behavior.