Add clamp functions #44097

Xaeroxe · Aug 26, 2017

Implementation of clamp feature:

Tracking issue: #44095
RFC: rust-lang/rfcs#1961

rust-highfive · Aug 26, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BurntSushi (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

scottmcm · Aug 26, 2017

scottmcm reviewed Aug 26, 2017

View changes

scottmcm · Aug 26, 2017

src/libstd/f32.rs

+    /// 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);
+    /// ```


Perhaps an example of NAN input giving NAN output?

scottmcm · Aug 26, 2017

src/libsyntax/feature_gate.rs

@@ -372,6 +372,9 @@ declare_features! (

    // #[doc(cfg(...))]
    (active, doc_cfg, "1.21.0", Some(43781)),
+
+    // Provide clamp functions
+    (active, clamp, "1.21.0", Some(44095)),


No need for the feature gate in code for lib changes; the attribute is sufficient.

lukaramu · Aug 26, 2017

lukaramu reviewed Aug 26, 2017

View changes

lukaramu · Aug 26, 2017

src/libstd/f32.rs

@@ -1080,6 +1080,27 @@ impl f32 {
        0.5 * ((2.0 * self) / (1.0 - self)).ln_1p()
    }

+    /// 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.


Should this be "is NaN" and not "equals NaN"? Nothing is equal (as in ==) to NaN, not even NaN.

lukaramu · Aug 26, 2017

src/libstd/f64.rs

@@ -970,6 +970,27 @@ impl f64 {
        0.5 * ((2.0 * self) / (1.0 - self)).ln_1p()
    }

+    /// 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.


Same as above.

carols10cents · Aug 28, 2017

Thanks for the PR! We’ll periodically check in on it to make sure that @BurntSushi or someone else from the team reviews it soon.

BurntSushi · Aug 28, 2017

BurntSushi reviewed Aug 28, 2017

View changes

BurntSushi · Aug 28, 2017

src/libcore/cmp.rs

@@ -481,6 +481,32 @@ pub trait Ord: Eq + PartialOrd<Self> {
    where Self: Sized {
        if self <= other { self } else { other }
    }
+
+    /// 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.


I think this should follow our doc formatting conventions, no? I think we usually have a panics header for documenting panic'ing conditions.

BurntSushi · Aug 28, 2017

BurntSushi reviewed Aug 28, 2017

View changes

BurntSushi · Aug 28, 2017

src/libcore/cmp.rs

+        } else {
+            self
+        }
+    }


Could you format this in a way that is consistent with the surrounding code? (Do we do rustmt on this stuff yet?)

BurntSushi · Aug 28, 2017

BurntSushi reviewed Aug 28, 2017

View changes

BurntSushi · Aug 28, 2017

src/libstd/f32.rs

@@ -1080,6 +1080,29 @@ impl f32 {
        0.5 * ((2.0 * self) / (1.0 - self)).ln_1p()
    }

+    /// 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 is NaN, or max is NaN.


Needs panic header for here too.

BurntSushi · Aug 28, 2017

BurntSushi reviewed Aug 28, 2017

View changes

BurntSushi · Aug 28, 2017

src/libstd/f64.rs

@@ -970,6 +970,29 @@ impl f64 {
        0.5 * ((2.0 * self) / (1.0 - self)).ln_1p()
    }

+    /// 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 is NaN, or max is NaN.


Needs panic header.

BurntSushi · Aug 28, 2017

BurntSushi requested changes Aug 28, 2017

View changes

This mostly looks good, but have some nits! I guess this should also technically wait to merge until the RFC has been merged?

Xaeroxe · Sep 1, 2017

r? @BurntSushi

Yeah this will have to wait for the RFC to be merged. I mostly opened this early since the implementation was so straightforward.

BurntSushi · Sep 1, 2017

BurntSushi requested changes Sep 1, 2017

View changes

Oh, and could you also add some unit tests for the panic conditions? Thanks!

aturon · Sep 6, 2017

@bors r=burntsushi

bors · Sep 6, 2017

📌 Commit b762283 has been approved by burntsushi

Xaeroxe · Sep 6, 2017

@aturon @BurntSushi That didn't seem to work :/

aturon · Sep 6, 2017

@Xaeroxe it's on the queue, which is pretty backlogged at the moment.

Mark-Simulacrum · Sep 6, 2017

@bors rollup

StefanoD · Sep 6, 2017

StefanoD reviewed Sep 6, 2017

View changes

StefanoD · Sep 6, 2017

src/libstd/f64.rs

+    /// Panics if min > max, min is NaN, or max is NaN.
+    #[unstable(feature = "clamp", issue = "44095")]
+    #[inline]
+    pub fn clamp(self, min: f64, max: f64) -> f64 {


I'm a Rust beginner, but I think, this should also work. I don't like the mut.

if x < min { min } if x > max { max } else { self }

That was the original implementation. This specific code pattern was chosen because it yields a highly efficient assembly implementation. This was actually the source of quite a bit of conversation before we settled on this.

Oh, interesting and let x?

Yeah, and that.

Holy caroly (this is actually no real word, but it rhymes)!

pcwalton · Sep 8, 2017

This broke Servo and Pathfinder. See servo/app_units#37

rust-highfive assigned BurntSushi Aug 26, 2017

This was referenced Aug 26, 2017

Open

Tracking issue for clamp RFC #44095

Merged

Add clamp RFC rust-lang/rfcs#1961

Xaeroxe force-pushed the Xaeroxe:clamp branch from eabd26e to eeb9a28 Aug 26, 2017

Add clamp functions

Loading status checks…

c589f86

Xaeroxe force-pushed the Xaeroxe:clamp branch from eeb9a28 to c589f86 Aug 26, 2017

Add NAN examples

Loading status checks…

f74c5d2

Xaeroxe added some commits Aug 26, 2017

Fix f32 examples.

Loading status checks…

61f20f8

Fix f64 examples

Loading status checks…

576426a

Xaeroxe changed the title ~~Add clamp functions and feature gate~~ Add clamp functions Aug 27, 2017

carols10cents added the S-waiting-on-review label Aug 28, 2017

BurntSushi added S-waiting-on-author and removed S-waiting-on-review labels Aug 28, 2017

Fix documentation and formatting.

Loading status checks…

2e34ff7

Xaeroxe force-pushed the Xaeroxe:clamp branch from bd556e7 to c3ff2d4 Sep 5, 2017

Add panic unit tests

Loading status checks…

b762283

Xaeroxe force-pushed the Xaeroxe:clamp branch from c3ff2d4 to b762283 Sep 5, 2017

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Sep 6, 2017

Rollup merge of rust-lang#44097 - Xaeroxe:clamp, r=burntsushi
Add clamp functions Implementation of clamp feature: Tracking issue: rust-lang#44095 RFC: rust-lang/rfcs#1961

98b1d8c

Mark-Simulacrum referenced this pull request Sep 6, 2017
Closed
Rollup of 22 pull requests #44369

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Sep 7, 2017

Rollup merge of rust-lang#44097 - Xaeroxe:clamp, r=burntsushi
Add clamp functions Implementation of clamp feature: Tracking issue: rust-lang#44095 RFC: rust-lang/rfcs#1961

44351ed

Mark-Simulacrum referenced this pull request Sep 7, 2017
Merged
Rollup of 23 pull requests #44380

bors merged commit b762283 into rust-lang:master Sep 7, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Xaeroxe deleted the Xaeroxe:clamp branch Sep 7, 2017

Xaeroxe restored the Xaeroxe:clamp branch Sep 8, 2017

rust-lang/rust

Join GitHub today

Add clamp functions #44097

Conversation

Xaeroxe commented Aug 26, 2017

rust-highfive assigned BurntSushi Aug 26, 2017

This comment has been minimized.

rust-highfive commented Aug 26, 2017

This was referenced Aug 26, 2017

Xaeroxe force-pushed the Xaeroxe:clamp branch from eabd26e to eeb9a28 Aug 26, 2017

scottmcm reviewed Aug 26, 2017 View changes

This comment has been minimized.

This comment has been minimized.

Xaeroxe force-pushed the Xaeroxe:clamp branch from eeb9a28 to c589f86 Aug 26, 2017

lukaramu reviewed Aug 26, 2017 View changes

This comment has been minimized.

lukaramu Aug 26, 2017 • edited

This comment has been minimized.

Xaeroxe added some commits Aug 26, 2017

Xaeroxe changed the title Add clamp functions and feature gate Add clamp functions Aug 27, 2017

carols10cents added the S-waiting-on-review label Aug 28, 2017

This comment has been minimized.

carols10cents commented Aug 28, 2017

BurntSushi reviewed Aug 28, 2017 View changes

This comment has been minimized.

BurntSushi reviewed Aug 28, 2017 View changes

This comment has been minimized.

BurntSushi reviewed Aug 28, 2017 View changes

This comment has been minimized.

BurntSushi reviewed Aug 28, 2017 View changes

This comment has been minimized.

BurntSushi requested changes Aug 28, 2017 View changes

BurntSushi left a comment

BurntSushi added S-waiting-on-author and removed S-waiting-on-review labels Aug 28, 2017

This comment has been minimized.

Xaeroxe commented Sep 1, 2017

BurntSushi requested changes Sep 1, 2017 View changes

BurntSushi left a comment

Xaeroxe force-pushed the Xaeroxe:clamp branch from bd556e7 to c3ff2d4 Sep 5, 2017

Xaeroxe force-pushed the Xaeroxe:clamp branch from c3ff2d4 to b762283 Sep 5, 2017

This comment has been minimized.

aturon commented Sep 6, 2017

This comment has been minimized.

bors commented Sep 6, 2017

This comment has been minimized.

Xaeroxe commented Sep 6, 2017

This comment has been minimized.

aturon commented Sep 6, 2017

This comment has been minimized.

Mark-Simulacrum commented Sep 6, 2017

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Sep 6, 2017

Mark-Simulacrum referenced this pull request Sep 6, 2017

bors added a commit that referenced this pull request Sep 6, 2017

StefanoD reviewed Sep 6, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors added a commit that referenced this pull request Sep 6, 2017

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Sep 7, 2017

Mark-Simulacrum referenced this pull request Sep 7, 2017

bors added a commit that referenced this pull request Sep 7, 2017

Hide details View details bors merged commit b762283 into rust-lang:master Sep 7, 2017 1 check passed

1 check passed

Xaeroxe deleted the Xaeroxe:clamp branch Sep 7, 2017

This comment has been minimized.

pcwalton commented Sep 8, 2017

Xaeroxe restored the Xaeroxe:clamp branch Sep 8, 2017

Xaeroxe force-pushed the Xaeroxe:clamp branch from `eabd26e` to `eeb9a28` Aug 26, 2017

scottmcm reviewed Aug 26, 2017

View changes

Xaeroxe force-pushed the Xaeroxe:clamp branch from `eeb9a28` to `c589f86` Aug 26, 2017

lukaramu reviewed Aug 26, 2017

View changes

lukaramu Aug 26, 2017 •

edited

BurntSushi reviewed Aug 28, 2017

View changes

BurntSushi reviewed Aug 28, 2017

View changes

BurntSushi reviewed Aug 28, 2017

View changes

BurntSushi reviewed Aug 28, 2017

View changes

BurntSushi requested changes Aug 28, 2017

View changes

BurntSushi requested changes Sep 1, 2017

View changes

Xaeroxe force-pushed the Xaeroxe:clamp branch from `bd556e7` to `c3ff2d4` Sep 5, 2017

Xaeroxe force-pushed the Xaeroxe:clamp branch from `c3ff2d4` to `b762283` Sep 5, 2017

StefanoD reviewed Sep 6, 2017

View changes

bors merged commit `b762283` into rust-lang:master Sep 7, 2017
1 check passed