Add clamp function #262
Add clamp function #262
Conversation
|
The build fails before it even starts compiling num. The build server is likely misconfigured. |
|
The 1.0 failure is #257 -- nothing you can can fix here. I'll give a review here in a little bit. |
|
Why does this require |
src/lib.rs
Outdated
| /// If input is less than min then min is returned, if input is greater than max then max is | ||
| /// returned. Otherwise input is returned. | ||
| #[inline] | ||
| pub fn clamp<T: PartialOrd + Copy>(input: T, min: T, max: T) -> T { |
There was a problem hiding this comment.
I agree with @clarcharr that this shouldn't need Copy.
src/lib.rs
Outdated
| /// returned. Otherwise input is returned. | ||
| #[inline] | ||
| pub fn clamp<T: PartialOrd + Copy>(input: T, min: T, max: T) -> T { | ||
| debug_assert!(min < max, "min must be less than max"); |
There was a problem hiding this comment.
I think min <= max is fine, if we're going to assert anything.
src/lib.rs
Outdated
| debug_assert!(min < max, "min must be less than max"); | ||
| if input <= min {min} | ||
| else if input >= max {max} | ||
| else {input} |
There was a problem hiding this comment.
This formatting is non-standard -- please run it through rustfmt.
I think it would be slightly preferable to return input when it is equal to the boundaries. This only matters for cases similar to stable sorting, where there may be internal data that's not part of the ordering. So just compare input < min and input > max.
|
Yeah good point. Copy made sense at the time but it's not really needed. Requested changes have been made. |
|
Another thing is that it'd be best to match on |
Why? Then it will require |
|
The alternative is partial_cmp but I don't understand what benefit we gain from using that. |
use std::cmp::Ordering;
match input.partial_cmp(&min) {
Some(Ordering::Less) => min,
_ =>
match input.partial_cmp(&max) {
Some(Ordering::Greater) => max,
_ => input
},
}This is my best attempt at implementing it using partial_cmp. I don't really like it. |
Make assert debug error more accurate.
|
@clarcharr See rust-lang/rust#39538, which found in that case that binary comparisons perform better than a ternary |
|
The suggestion would make sense if it were possible to implement it using just one match statement, but because it is being compared to two values, not one, the suggestion doesn't really work. |
Remove passive voice from documentation comments.
|
Merged - thanks! |
This PR adds a "clamp" function to num. Oftentimes it is desirable to restrict a value to between a certain minimum and maximum, the clamp function provides a generic way to do this.