Conversation
rustbot
added
S-waiting-on-review
|
r? @ibraheemdev rustbot has assigned @ibraheemdev. Use |
library/core/src/cmp/clamp.rs
Outdated
| fn clamp(self, value: $t) -> $t { | ||
| let (start, end) = self.into_inner(); | ||
| // Deliberately avoid using `clamp` to handle NaN consistently | ||
| value.max(start).min(end) |
There was a problem hiding this comment.
I would be very surprised if x.clamp_to(a..=b) were to behave differently at all from x.clamp(a, b), regardless of the various values for x, a, and b.
Isn't clamp moving toward making its NaN behaviour consistent, anyway?
There was a problem hiding this comment.
Yeah but imo it's worse if x.clamp_to(a..) and x.clamp_to(a..=b) handle nan differently. Making all of them panic on nan would be a plausible choice though. It's listed as an unresolved question on the tracking issue.
There was a problem hiding this comment.
Looking at the current code, it does appear that clamp simply panics on NaN, so, I would say that it's reasonable to always panic on NaN for these methods to match that behaviour. But I guess we can discuss that in the tracking issue instead of blocking this.
There was a problem hiding this comment.
(Side note: after a lot of poking around the changes to clamp and its original RFC, it appears that the panicking behaviour was explicitly decided as the best, and it was not an accident. So, I do think that panicking on NaN bounds like clamp does is the best behaviour.
There was a problem hiding this comment.
You're right, if it's named like clamp it should behave like clamp. I'm leaving the question as unresolved, though it's pretty likely that it'll end up this way.
I don't currently have any tests that it does panic on nan; should I add that, and if so where?
There was a problem hiding this comment.
I honestly would just look at whatever clamp's tests are and try to match them. Could even use some macros to just run the same tests on both functions to make sure the output matches.
| assert!(!self.start.is_nan(), "min was NaN"); | ||
| value.max(self.start) | ||
| } | ||
| } | ||
|
|
||
| #[unstable(feature = "clamp_bounds", issue = "147781")] | ||
| #[rustc_const_unstable(feature = "clamp_bounds", issue = "147781")] | ||
| impl const ClampBounds<$t> for RangeToInclusive<$t> { | ||
| fn clamp(self, value: $t) -> $t { | ||
| assert!(!self.end.is_nan(), "max was NaN"); |
There was a problem hiding this comment.
Minor nit, but since these are ranges, you should probably use the start/end naming instead.
Note that std::range::RangeToInclusive uses the name last to represent an inclusive end instead of end, but since these are just for the regular ops ranges, we should match the actual field name.
5 participants
Implements the revised version of #147781. Supersedes #147786.
Currently I restrict the ClampBounds trait using a second, perma-unstable feature. I don't know if that's the usual way to deal with this kind of traits, I'd be happy to change it if not.
I currently define NaN as equal to no bound. This is consistent withmaxandmin, but is inconsistent withclamp, which panics.Changed so that the float versions panic if any bound is NaN, just like
clampdoes.