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.

Sign up for GitHub

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

Jump to bottom

Tracking issue for clamp RFC #44095

Closed
3 tasks done
Xaeroxe opened this issue Aug 26, 2017 · 102 comments
Closed
3 tasks done

Tracking issue for clamp RFC #44095

Xaeroxe opened this issue Aug 26, 2017 · 102 comments
Labels
B-unstable Feature: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Xaeroxe
Copy link
Contributor

Xaeroxe commented Aug 26, 2017 •
edited

Tracking issue for rust-lang/rfcs#1961

PR here: #44097 #58710
Stabilization PR: #77872

TODO:

  • Have RFC pass final comment period
  • Implement RFC
  • Stabilize
This was referenced Aug 26, 2017
Merged
Merged
@Mark-Simulacrum Mark-Simulacrum added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 26, 2017
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Sep 6, 2017
@Mark-Simulacrum
Rollup merge of rust-lang#44097 - Xaeroxe:clamp, r=burntsushi
98b1d8c 
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
@Mark-Simulacrum
Rollup merge of rust-lang#44097 - Xaeroxe:clamp, r=burntsushi
44351ed 
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
@pcwalton
Explicitly invoke Au::clamp() to avoid colliding with the unstable …
7136c16 
…Rust library method of the

same name.

Fixes breakage from rust-lang/rust#44095.
@pcwalton pcwalton mentioned this issue Sep 8, 2017
Merged
@pcwalton

This comment has been minimized.

Sign in to view
bors-servo pushed a commit to servo/app_units that referenced this issue Sep 8, 2017
Auto merge of #37 - pcwalton:clamp, r=Manishearth
26d07cd 
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
Copy link
Member

aturon 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?

@BurntSushi
Copy link
Member

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
Copy link
Contributor Author

Xaeroxe 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
Copy link
Member

aturon 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
Copy link
Contributor Author

Xaeroxe commented Sep 8, 2017

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

@aturon
Copy link
Member

aturon 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
Copy link
Contributor Author

Xaeroxe commented Sep 8, 2017

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

@Xaeroxe Xaeroxe mentioned this issue Sep 8, 2017
Merged
@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Sep 8, 2017

PR ready #44438

bors added a commit that referenced this issue Sep 9, 2017
@bors
Auto merge of #44438 - Xaeroxe:clamp, r=Mark-Simulacrum
929b878 
Revert clamp

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

egilburg commented Sep 9, 2017 •
edited

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
Copy link
Contributor Author

Xaeroxe commented Sep 9, 2017

I suppose that idea mandates an RFC.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe 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 as completed Sep 9, 2017
@Xaeroxe
Copy link
Contributor Author

Xaeroxe 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
Copy link
Member

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
Copy link
Contributor

CryZe commented Sep 11, 2017 •
edited

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
Copy link
Contributor Author

Xaeroxe 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.

@KodrAus KodrAus added Libs-Tracked Libs issues that are tracked on the team's project board. and removed disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jul 29, 2020
@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Sep 9, 2020

This has been merged and unstable for about a year and a half now. How do we feel about stabilizing this?

@EdorianDark
Copy link
Contributor

I would love stabilizing this!

@KamilaBorowska
Copy link
Contributor

KamilaBorowska commented Sep 18, 2020 •
edited

If clamp method name conflict sounds like an issue, I suggested changing name resolution at one point in #66852 (comment), and it would help with this too.

@matklad
Copy link
Member

matklad commented Oct 11, 2020

@Xaeroxe I think the process is to submit stabilization PR and ask for libs team consensus on that. It seems that t-libs is overloaded and can't keep up with non-fcped things.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Oct 12, 2020

Okay, did that #77872

@Xaeroxe Xaeroxe mentioned this issue Oct 12, 2020
Merged
@kennytm
Copy link
Member

kennytm commented Oct 16, 2020

@matklad actually an FCP proposal already started last year at #44095 (comment), but it is stuck because there is one remaining checkbox.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Oct 16, 2020

In that case, I think being pinged about once a year on an issue is pretty tolerable.

@Kimundi
@sfackler
@withoutboats

#44095 (comment) is still awaiting your attention

@LukasKalbertodt
Copy link
Member

The libs team has changed quite a bit since the FCP was started. What do you all think about just starting a new FCP in the stabilization PR? Feels like that shouldn't take longer than waiting for the remaining checkboxes here.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Oct 29, 2020

@LukasKalbertodt fine by me, do you mind kicking that off?

@m-ou-se
Copy link
Member

m-ou-se commented Nov 11, 2020

Cancelling the FCP here, because that FCP now happened on the stabilization PR: #77872 (comment)

@Fcpbot cancel

@m-ou-se
Copy link
Member

m-ou-se commented Nov 11, 2020

uh

@rfcbot cancel

@rfcbot
Copy link

rfcbot commented Nov 11, 2020

@m-ou-se proposal cancelled.

bors added a commit to rust-lang-ci/rust that referenced this issue Nov 22, 2020
@bors
Auto merge of rust-lang#77872 - Xaeroxe:stabilize-clamp, r=scottmcm
5d5ff84 
Stabilize clamp

Tracking issue: rust-lang#44095

Clamp has been merged and unstable for about a year and a half now. How do we feel about stabilizing this?
@jplatte
Copy link
Contributor

jplatte commented Nov 23, 2020

Stabilized in #77872, I think this can be closed.

@m-ou-se m-ou-se closed this as completed Nov 23, 2020
@m-ou-se m-ou-se mentioned this issue Nov 27, 2020
Merged
@vicky5124 vicky5124 mentioned this issue Dec 6, 2020
Closed
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 19, 2020
@bors
Auto merge of rust-lang#79473 - m-ou-se:clamp-in-core, r=m-ou-se
c1d5843 
Move {f32,f64}::clamp to core.

`clamp` was recently stabilized (tracking issue: rust-lang#44095). But although `Ord::clamp` was added in `core` (because `Ord` is in `core`), the versions for the `f32` and `f64` primitives were added in `std` (together with `floor`, `sin`, etc.), not in `core` (together with `min`, `max`, `from_bits`, etc.).

This change moves them to `core`, such that `clamp` on floats is available in `no_std` programs as well.
@parity-benchapp parity-benchapp bot mentioned this issue Mar 10, 2021
Merged
@mockersf mockersf mentioned this issue Mar 13, 2021
Closed
bors bot pushed a commit to bevyengine/bevy that referenced this issue Mar 13, 2021
@mockersf
use std clamp instead of Bevy's (#1644)
75ae20d 
Rust std's `clamp` has been stabilised in 1.50: rust-lang/rust#44095

This is already the minimum supported version, so no change there 👍
@TheHunterDog TheHunterDog mentioned this issue Mar 26, 2021
Closed
@afalout afalout mentioned this issue Apr 24, 2021
Closed
1 task
@m-ou-se m-ou-se mentioned this issue Jun 19, 2021
Closed
@LetsMelon LetsMelon mentioned this issue Jul 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
B-unstable Feature: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Rollup of 23 pull requests kennytm/rust