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

Tracking issue for clamp RFC #44095

Open
Xaeroxe opened this issue Aug 26, 2017 · 51 comments

Comments

Projects
None yet
@Xaeroxe
Copy link
Contributor

commented Aug 26, 2017

Tracking issue for rust-lang/rfcs#1961

PR here: #44097 #58710

TODO:

  • Have RFC pass final comment period
  • Implement RFC

This was referenced Aug 26, 2017

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue 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

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue 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

pcwalton added a commit to pcwalton/app_units that referenced this issue Sep 8, 2017

Explicitly invoke `Au::clamp()` to avoid colliding with the unstable …
…Rust library method of the

same name.

Fixes breakage from rust-lang/rust#44095.
@pcwalton

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

Please note: This broke Servo and Pathfinder.

bors-servo added a commit to servo/app_units that referenced this issue Sep 8, 2017

Auto merge of #37 - pcwalton:clamp, r=Manishearth
Explicitly invoke `Au::clamp()` to avoid colliding with the unstable Rust library method of the same name.

Fixes breakage from rust-lang/rust#44095.

r? @Manishearth
@aturon

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

cc @rust-lang/libs, this is a case similar to min/max, where the ecosystem was already using the clamp name, and hence adding it has caused ambiguity. This is permitted breakage per semver policy, but it's nevertheless causing downstream pain.

Nominating for the triage meeting on Tues.

Any thoughts in the meantime?

@aturon aturon added the I-nominated label Sep 8, 2017

@BurntSushi

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

I'm kind of with @bluss on this one in that it would be nice not to repeat it. "Clamp" is probably a great name, but could we sidestep this by choosing a different name?

@Xaeroxe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2017

restrict
clamp_to_range
min_max (Because it's kind of like combining min and max.)
These might work. Can we use crater to determine how bad the impact of clamp actually is? clamp is well recognized across several languages and libraries.

@aturon

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

If we think we might need to rename, it's probably best to revert the PR immediately, and then test more carefully with crater etc. @Xaeroxe, up for that?

@Xaeroxe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2017

Sure. I've never used crater before, but I can learn.

@aturon

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

@Xaeroxe ah sorry, I meant getting a revert PR up quickly. (I'm on vacation today so you may need someone else on libs, like @BurntSushi or @alexcrichton, to help land it).

@Xaeroxe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2017

I'm preparing the PR now. Have fun on your vacation!

@Xaeroxe Xaeroxe referenced this issue Sep 8, 2017

Merged

Revert clamp #44438

@Xaeroxe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2017

PR ready #44438

bors added a commit that referenced this issue Sep 9, 2017

Auto merge of #44438 - Xaeroxe:clamp, r=Mark-Simulacrum
Revert clamp

Revert clamp per #44095 (comment) while we take time to assess the potential backwards compatibility damage done by it.
@egilburg

This comment has been minimized.

Copy link

commented Sep 9, 2017

Could clamp_to_range(min, max) be composed of clamp_to_min(min) and clamp_to_max(max) (with the additional assertion that min <= max), but those functions could also be called independently?

@Xaeroxe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2017

I suppose that idea mandates an RFC.

@Xaeroxe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2017

I gotta say though I've been working on getting a 4 line function into the std library for 6 months now. I'm kind of worn out. The same function got merged into num in 2 days and that's good enough for me. If anyone else really wants this in the std library go ahead, but I'm just not ready for another 6 months of this.

@Xaeroxe Xaeroxe closed this Sep 9, 2017

@Xaeroxe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2017

I'm reopening this so that @aturon 's previous nomination will still be seen.

@Xaeroxe Xaeroxe reopened this Sep 11, 2017

@scottmcm

This comment has been minimized.

Copy link
Member

commented Sep 11, 2017

I think that either this should go in as-written or the guidance on what changes can be made should be updated to avoid wasting peoples' time in future.

It was very clear from early that this could cause the breakage it did. Personally, I compared it to ord_max_min which broke a bunch of things:

And the response to that was "The function Ord::min was added [...] The libs team decided today that this is accepted breakage". And that was a TMTOWTDI feature with a more-common name, whereas clamp didn't already exist in std under a different form.

It feels, subjectively, to me that if this RFC is reverted, the actual rule is "You basically can't put new methods on traits in std, except maybe Iterator".

@CryZe

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2017

You also can't really put new methods on actual types either. Consider the situation where someone had an "extension trait" for a type in std. Now std implements a method the extension trait provided as an actual method on this type. Then this reaches stable, but this new method is still behind a feature flag. The compiler will then complain that the method is behind a feature flag and can't be used with the stable toolchain, instead of the compiler choosing the extension trait's method like before and thus causing breakage on the stable compiler.

@Xaeroxe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2017

It's also worth noting: This isn't just a standard library problem. Method call syntax makes it really difficult to avoid introducing breaking changes just about anywhere in the ecosystem.

@EdorianDark

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

I think, that option 3 is the best option.

@lu-zero

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

1 or 2 are the least surprising. I'd stay with 1 since lots of code would have less to do to replace the local implementation with the std one.

@scottmcm

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

I think we should either plan to use all the range* types or none of them.

Of course, that's harder for things like Range than for RangeInclusive. But there's something nice about (0.0..1.0).clamp(2.0_f32) => 0.99999994_f32.

@EdorianDark

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2018

@kennytm So if I would open a pull request with option 3 do you think it would get merged?
Or what do you think about how to proceed next?

@kennytm

This comment has been minimized.

Copy link
Member

commented Sep 15, 2018

@EdorianDark For this we'll need to ask @rust-lang/libs 😃

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2018

I personally like option 2, with RangeInclusive only. As mentioned "partial clamping" already exist with min and max.

@jminer

This comment has been minimized.

Copy link

commented Sep 17, 2018

I agree with @SimonSapin, although I would also be OK with option 1. With option 3, I likely wouldn't use the function because it seems backwards to me. In the other languages/libraries with clamp that @kennytm surveyed earlier, 5 out of 7 (all but Swift and Qt) have the value first, then the range.

kennytm added a commit to kennytm/rust that referenced this issue Mar 11, 2019

Rollup merge of rust-lang#58710 - EdorianDark:master, r=sfackler
Add clamp for ranges. Implements rust-lang#44095

Does not build, but I can not figure out why.
Either I import ops and get the error:

``
error: unused import: `core::ops::RangeInclusive`
  --> src/libstd/f64.rs:16:5
   |
16 | use core::ops::RangeInclusive;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
``

or I do not import and get another error

``
error[E0412]: cannot find type `RangeInclusive` in this scope=======>  ] 35/36: std
   --> src/libstd/f64.rs:928:32
    |
928 |     pub fn clamp(self, range : RangeInclusive<Self>) -> Self
    |                                ^^^^^^^^^^^^^^ not found in this scope
help: possible candidate is found in another module, you can import it into scope
    |
12  | use core::ops::RangeInclusive;
    |
``

If it would build, could it get merged?
Can someone figure out, why it doesn't build?

bors added a commit that referenced this issue Mar 12, 2019

Auto merge of #58710 - EdorianDark:master, r=sfackler
Add clamp for ranges. Implements #44095

Does not build, but I can not figure out why.
Either I import ops and get the error:

``
error: unused import: `core::ops::RangeInclusive`
  --> src/libstd/f64.rs:16:5
   |
16 | use core::ops::RangeInclusive;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
``

or I do not import and get another error

``
error[E0412]: cannot find type `RangeInclusive` in this scope=======>  ] 35/36: std
   --> src/libstd/f64.rs:928:32
    |
928 |     pub fn clamp(self, range : RangeInclusive<Self>) -> Self
    |                                ^^^^^^^^^^^^^^ not found in this scope
help: possible candidate is found in another module, you can import it into scope
    |
12  | use core::ops::RangeInclusive;
    |
``

If it would build, could it get merged?
Can someone figure out, why it doesn't build?

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Mar 14, 2019

Rollup merge of rust-lang#58710 - EdorianDark:master, r=sfackler
Add clamp for ranges. Implements rust-lang#44095

Ready for merge

bors added a commit that referenced this issue Mar 14, 2019

Auto merge of #58710 - EdorianDark:master, r=sfackler
Add clamp for ranges. Implements #44095

Ready for merge

bors added a commit that referenced this issue Mar 15, 2019

Auto merge of #58710 - EdorianDark:master, r=sfackler
Add clamp for ranges. Implements #44095

Ready for merge
@EdorianDark

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

Clamp is now in master again!

@Xaeroxe

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

I'm pleased, though I'm still trying to figure out what changed that made this acceptable now, whereas it wasn't in #44097

@kennytm

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

We've now got a warning period due to #48552, instead of instantly breaking inference even before stabilizing.

@Xaeroxe

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

That's great news, thank you!

@Xaeroxe

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@kennytm I just want to thank you for the legwork you did on making #48552 happen, and @EdorianDark thanks for your interest in this and getting it implemented. It's wonderful to see this finally merged.

@kornelski

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

https://rust.godbolt.org/z/JmLWJi

pub fn clamped(a: f32) -> f32 {
   a.clamp(0.,255.)
}

Compiles to:

  vxorps xmm1, xmm1, xmm1
  vmaxss xmm0, xmm1, xmm0
  vmovss xmm1, dword ptr [rip + .LCPI0_0]
  vminss xmm0, xmm1, xmm0

which isn't too bad (vmaxss and vminss are used), but:

pub fn maxmined(a: f32) -> f32 {
   (0f32).max(a).min(255.)
}

uses one instruction less:

  vxorps xmm1, xmm1, xmm1
  vmaxss xmm0, xmm0, xmm1
  vminss xmm0, xmm0, dword ptr [rip + .LCPI1_0]

Is that inherent to the clamp implementation, or just a quirk of LLVM optimization?

@scottmcm

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

@kornelski clamping a NAN is supposed to preserve that NAN, which that maxmined doesn't, because max/min preserve the non-NAN.

It'd be great to find an implementation that both meets the NAN expectations and is shorter. And it would be good for the doctests to showcase NAN handling. Looks like the original PR had some:

rust/src/libstd/f32.rs

Lines 1089 to 1094 in b762283

/// #![feature(clamp)]
/// use std::f32::NAN;
/// 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);
/// assert!((NAN).clamp(-2.0f32, 1.0f32).is_nan());

@Xaeroxe

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@tspiteri

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

Why does clamping floats panic if min or max is NaN? I would change the assertion from assert!(min <= max) to assert!(!(min > max)), so that a NaN minimum or maximum would have no effect, just like in the max and min methods.

@Xaeroxe

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

NAN for min or max in clamp is likely indicative of a programming error, and we figured it was better to panic sooner rather than possibly feeding unclamped data out to IO. If you don't want an upper or lower bound this function isn't for you.

@jdahlstrom

This comment has been minimized.

Copy link

commented Mar 23, 2019

You could always use INF and -INF if you don't want an upper or lower bound, right? Which also makes mathematical sense, unlike NaN. But most of the time it's better to use max and min for that.

@EdorianDark

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

@Xaeroxe Thank you for the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.