Simplify color component rounding in parser #43
Conversation
The cast to `u8` is saturating, so some bounds checking can be dropped. The division and multiplication in `value / 100.0 * 255.0` are not collapsed, so it has the same rounding behavior as when using `parse_number_or_percent`.
There was a problem hiding this comment.
I agree with the analysis.
There are additional is_finite assertions in the old version, but hopefully the parse would have failed if it hit NaN? I think the one case which could be problematic is if we parse a floating point number which is infinity (e.g. 10e50000 or w/e). Maybe it's worth a test to make sure this has the same behaviour in that case?
Edit: But of course, we want that to clamp, which will be the case with this new behaviour.
|
This code is all going away in the. next breaking release (due to the |
|
(That's more of an FYI / reminder. Not saying I disapprove of the changes.) |
waywardmonkeys
left a comment
waywardmonkeys
left a comment
There was a problem hiding this comment.
There are 2 more instances of this code pattern with the alpha here and hsla parsing which could be adjusted similarly.
src/color.rs
Outdated
| color.red = from_percent(value / 100.0); | ||
| color.green = from_percent(self.parse_list_number_or_percent()?); | ||
| color.blue = from_percent(self.parse_list_number_or_percent()?); | ||
| color.red = (value / 100.0 * 255.0).round() as u8; |
There was a problem hiding this comment.
I sort of feel like extra parens here would be nice for my braindead days.
tomcur
changed the title
tomcur
force-pushed
the
simplify-bounds
branch
from
d52fe21 to
7edcce9
Compare
The cast to
u8is saturating, so some bounds checking can be dropped.The division and multiplication in
value / 100.0 * 255.0are not collapsed, so it has the same rounding behavior as when usingparse_number_or_percent.