-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add clamp RFC #1961
Merged
Merged
Add clamp RFC #1961
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
fc9dad4
Add clamp RFC
Xaeroxe 3988071
Remove incorrect examples
Xaeroxe ba9f88d
Move RFC into text and remove RangeInclusive use
Xaeroxe 1a6a796
Revise NAN behavior & optimize handling of floats.
Xaeroxe 0c1dbc5
Update 0000-clamp.md
Xaeroxe b16f5bd
Update RFC to panic if max is less than min.
Xaeroxe 2e31a67
Fix float impl
Xaeroxe b2df178
Update RFC to reflect discussion.
Xaeroxe 81fb090
Return None if min can't be compared to max.
Xaeroxe acacdba
Fix implementation
Xaeroxe 2abd671
Fix doc string
Xaeroxe 5c58c29
Remove edge cases table as it is now redundant.
Xaeroxe 2923d07
Remove partial_clamp
Xaeroxe f2cc3c0
Remove partial_clamp from alternatives section
Xaeroxe 8e96822
Remove outdated text
Xaeroxe 71d594d
Update RFC to better reflect conversation thus far
Xaeroxe ac69333
RFC 1961 introduces clamp functions
aturon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,155 @@ | ||
- Feature Name: clamp functions | ||
- Start Date: 2017-03-26 | ||
- RFC PR: (leave this empty) | ||
- Rust Issue: (leave this empty) | ||
|
||
# Summary | ||
[summary]: #summary | ||
|
||
Add functions to the language which take a value and an inclusive range, and will "clamp" the input to the range. I.E. | ||
|
||
```Rust | ||
if input > max { | ||
return max; | ||
} | ||
else if input < min { | ||
return min; | ||
} else { | ||
return input; | ||
} | ||
``` | ||
|
||
Likely locations would be in std::cmp::clamp implemented for all Ord types, and a special version implemented for f32 and f64. | ||
The f32 and f64 versions could live either in std::cmp or in the primitive types themselves. There are good arguments for either | ||
location. | ||
|
||
# Motivation | ||
[motivation]: #motivation | ||
|
||
Clamp is a very common pattern in Rust libraries downstream. Some observed implementations of this include: | ||
|
||
http://nalgebra.org/rustdoc/nalgebra/fn.clamp.html | ||
|
||
http://rust-num.github.io/num/num/fn.clamp.html | ||
|
||
Many libraries don't expose or consume a clamp function but will instead use patterns like this: | ||
```Rust | ||
if input > max { | ||
max | ||
} | ||
else if input < min { | ||
min | ||
} else { | ||
input | ||
} | ||
``` | ||
and | ||
```Rust | ||
input.max(min).min(max); | ||
``` | ||
and even | ||
```Rust | ||
match input { | ||
c if c > max => max, | ||
c if c < min => min, | ||
c => c, | ||
} | ||
``` | ||
|
||
Typically these patterns exist where there is a need to interface with APIs that take normalized values or when sending | ||
output to hardware that expects values to be in a certain range, such as audio samples or painting to pixels on a display. | ||
|
||
While this is pretty trivial to implement downstream there are quite a few ways to do it and just writing the clamp | ||
inline usually results in rather a lot of control flow structure to describe a fairly simple and common concept. | ||
|
||
# Detailed design | ||
[design]: #detailed-design | ||
|
||
Add the following to std::cmp | ||
|
||
```Rust | ||
use ops::RangeInclusive; | ||
/// Returns the upper bound of the range if input is greater than the range, and the lower bound of | ||
/// the range if input is less than the range. Otherwise this will return input. | ||
#[inline] | ||
pub fn clamp<T: Ord>(input: T, range: RangeInclusive<T>) -> T { | ||
if let RangeInclusive::NonEmpty{start, end} = range { | ||
if input < start { | ||
return start; | ||
} | ||
else if input > end { | ||
return end; | ||
} | ||
else { | ||
return input; | ||
} | ||
} | ||
else { | ||
// This should never be executed. | ||
return input; | ||
} | ||
} | ||
``` | ||
|
||
And the following to libstd/f32.rs, and a similar version for f64 | ||
|
||
```Rust | ||
use ops::RangeInclusive; | ||
/// Returns the upper bound if self is greater than the bound, and the lower bound if self is less than the bound. | ||
/// Otherwise this returns self. | ||
/// | ||
/// # Examples | ||
/// | ||
/// ``` | ||
/// assert!((-3.0f32).clamp(-2.0f32...1.0f32) == -2.0f32); | ||
/// assert!((0.0f32).clamp(-2.0f32...1.0f32) == 0.0f32); | ||
/// assert!((2.0f32).clamp(-2.0f32...1.0f32) == 1.0f32); | ||
/// ``` | ||
#[inline] | ||
pub fn clamp(self, range: RangeInclusive<f32>) -> f32 { | ||
if let NonEmpty{start, end} = range { | ||
if self < start { | ||
return start; | ||
} | ||
else if self > max { | ||
return max; | ||
} | ||
else { | ||
return self; | ||
} | ||
} | ||
else { | ||
// This should never be executed. | ||
return NAN; | ||
} | ||
} | ||
``` | ||
|
||
There are 3 special float values the clamp function will need to handle, and 3 positions into which they can go so I will represent | ||
the edge case behavior with a 3x3 chart. | ||
|
||
| |INFINITY|NEG_INFINITY|NAN| | ||
|---|---|---|---| | ||
|self|return max;|return min;|return NAN;| | ||
|upper bound|No upper bound enforced|return NEG_INFINITY;|No upper bound enforced| | ||
|lower bound|return INFINITY;|No lower bound enforced|No lower bound enforced| | ||
|
||
# How We Teach This | ||
[how-we-teach-this]: #how-we-teach-this | ||
|
||
The proposed changes would not mandate modifications to any Rust educational material. | ||
|
||
# Drawbacks | ||
[drawbacks]: #drawbacks | ||
|
||
This is trivial to implement downstream, and several versions of it exist downstream. | ||
|
||
# Alternatives | ||
[alternatives]: #alternatives | ||
|
||
Alternatives were explored at https://internals.rust-lang.org/t/clamp-function-for-primitive-types/4999 | ||
|
||
# Unresolved questions | ||
[unresolved]: #unresolved-questions | ||
|
||
Should the float version of the clamp function live in f32 and f64, or in std::cmp as that's where the Ord version would go? |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should tie the stability of this RFC with #1192 (rust-lang/rust#28237) here by using
range: RangeInclusive<T>
instead ofmin: T, max: T
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After messing around with the playground a bit and doing some thinking I actually agree with you. I much prefer the RangeInclusive syntax but the instability of it is too much of a drawback.