Skip to content
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 17 commits into from
Sep 6, 2017
Merged
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 129 additions & 0 deletions text/0000-clamp.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
- 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 on the Ord trait, 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably update this now that the RFC has made a decision on where.


# 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::Ord

```Rust
/// Returns max if self is greater than max, and min if self is less than min.
/// Otherwise this will return self. Panics if min > max.
#[inline]
pub fn clamp(self, min: Self, max: Self) -> Self {
assert!(min <= max);
if self < min {
min
}
else if self > max {
max
} else {
self
}
}
```

And the following to libstd/f32.rs, and a similar version for f64

```Rust
/// Returns max if self is greater than max, and min if self is less than min.
/// Otherwise this returns self. Panics if min > max, min equals NAN, or max equals NAN.
///
/// # 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);
/// ```
pub fn clamp(self, min: f32, max: f32) -> f32 {
assert!(min <= max);
let mut x = self;
if x < min { x = min; }
if x > max { x = max; }
x
}
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative implementation which results in better code is:

pub fn clamp(input:f32, min: f32, max: f32) -> f32 {
    let mut x = input;
    if !(x < min) { x = min; }
    if !(x > max) { x = max; }
    x
}

It conveniently preserves the source when it is NaN, as required in the edge cases listed below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks great!

Copy link
Contributor

@ranma42 ranma42 Mar 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... but it incorrectly returns NaN when max == NaN || min == NaN (EDIT: this is pseudocode... as crazy as it looks, the check for NaN would be max != max || min != min).
I am unsure if this is good or bad: while the infinity corresponding to no enforcement is straightforward, it is not obvious to me what is the desirable behaviour for NaN.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know honestly that might be better behavior than what I proposed. It assumes a NaN is unintentional which they often are. If someone explicitly wants no bounds enforced they should provide infinity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranma42 I think you mean if !(x >= min) etc.


# 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

Additionally there is the option of placing clamp in std::cmp in order to avoid backwards compatibility problems. This is however semantically undesirable, as `1.clamp(2, 3);` is more readable than `clamp(1, 2, 3);`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding min and max as methods on Ord caused a number of regressions (rust-lang/rust#42496 (comment)), and I'm unsure what their fate will be. Do we expect clamp to hit similar things?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scottmcm Possibly, although to a much lesser magnitude I think. I guess the only way to know is to try it and see.


# Unresolved questions
[unresolved]: #unresolved-questions

Is the proposed handling for NAN inputs ideal?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was actually resolved (some of the inputs: https://internals.rust-lang.org/t/clamp-function-for-primitive-types/4999/19 #1961 (comment) #1961 (comment)); it might be worth a note in the RFC explaining why the specified NaN behaviour was chosen.