Clamp function for primitive types

language design
21

This alternative produces slightly better code (no movaps):

minsd	%xmm1, %xmm0
maxsd	%xmm2, %xmm0
2 Likes
23

Nice, it works well (With the typo of < > exchange fixed), but it has different behaviour for NaN start/end (not a useful behaviour). Thatā€™s a legitimate design, but itā€™s something to take into account.

24

Yes, I was adding a post about that and discussing the edge cases, but I realized that most of it was already in the RFC, so I wrote there :slight_smile:

25

IME if youā€™re actually trying to use NaN for something constructive, you want it to be propagated by everything. That is, NaN.clamp(lo, hi) should return the NaN (and so should min(NaN, x) and max(NaN, x) ā€“ Iā€™m surprised to hear that they donā€™t).

The behavior of NaN in the lo and hi arguments matters considerably less, because those are much less likely to be data.

(I do not know whether this corresponds to the behavior of the x86 instructions mentioned above.)

26

Hi. Since this was just merged (1 day ago), it causes a compilation error when building latest firefox (from the mercurial repo) as follows:

 0:05.92 error: use of unstable library feature 'clamp' (see issue #44095)
 0:05.92    --> /home/xftroxgpx/build/1packages/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/third_party/rust/app_units/src/app_unit.rs:192:22
 0:05.92     |
 0:05.92 192 |         *self = self.clamp()
 0:05.92     |                      ^^^^^
 0:05.92     |
 0:05.92     = help: add #![feature(clamp)] to the crate attributes to enable
 0:05.92 
 0:05.92 error[E0061]: this function takes 2 parameters but 0 parameters were supplied
 0:05.92    --> /home/xftroxgpx/build/1packages/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/third_party/rust/app_units/src/app_unit.rs:192:22
 0:05.92     |
 0:05.92 192 |         *self = self.clamp()
 0:05.93     |                      ^^^^^ expected 2 parameters
 0:05.93 
 0:05.93 error[E0308]: mismatched types
 0:05.93    --> /home/xftroxgpx/build/1packages/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/third_party/rust/app_units/src/app_unit.rs:192:17
 0:05.93     |
 0:05.93 192 |         *self = self.clamp()
 0:05.93     |                 ^^^^^^^^^^^^ expected struct `app_unit::Au`, found mutable reference
 0:05.93     |
 0:05.93     = note: expected type `app_unit::Au`
 0:05.93                found type `&mut app_unit::Au`
 0:05.93     = help: here are some functions which might fulfill your needs:
 0:05.93             - .abs()
 0:05.94             - .clamp()
 0:05.94 
 0:05.95 error: aborting due to 3 previous errors
 0:05.95 
 0:06.38 error: Could not compile `app_units`.
 0:06.38

However clamp is (re?)defined internally:

impl Au {
    /// FIXME(pcwalton): Workaround for lack of cross crate inlining of newtype structs!
    #[inline]
    pub fn new(value: i32) -> Au {
        Au(value).clamp()
    }

    #[inline]
    fn clamp(self) -> Self {
        if self.0 > MAX_AU.0 {
            MAX_AU
        } else if self.0 < MIN_AU.0 {
            MIN_AU
        } else {
            self
        }
    }

    #[inline]
    fn clamp_self(&mut self) {
        *self = self.clamp()
    }

Iā€™m sure this will be fixed in app_units when the time comes, but I just wanted to mention it.

EDIT: Thanks for the fix (I have, for now, manually tested it to successfully compile latest firefox from mercurial repo; the pull for this to work auto seems on its way)

27

Crap this could be a huge problem for stabilizing this function. Itā€™s going to collide with several downstream implementations because after all this is a really useful function. This proves the std library would benefit from it but itā€™s also going to be difficult to introduce this now because itā€™s going to collide with the downstream implementations.

1 Like
28

What would it take to get this moved into the next epoch?

29

As I understand it there are no plans around how it would be possible to have varying APIs across epochs yet, from the epoch RFC:

More generally, breaking changes to the standard library are not possible.

30

So I guess the standard library probably just isnā€™t ever going to have a clamp function.

31

Considering how breakage around Ord::{min, max} are resolved, the clamp function will likely stay.

2 Likes
32

Having been hit by min/max breakage, Iā€™d vote to not repeat that.

33

I donā€™t know if this is within my permissions or not but Iā€™d like to run crater to see how this actually impacts downstream, and how that compares to the impact from min/max. Iā€™ll probably do that sometime this weekend.

34

FWIW, thereā€™s also num_traits::clamp (aka num::clamp) for any PartialOrd type. Since this is a standalone function, it wonā€™t conflict with having clamp methods on any type or trait.

35

Yeah I wrote that one too. It works I just think something like this could also do well in the standard library. Improvements have been made to the idea for the std library version as well. I guess at the end of the day itā€™s not critical this make it to stable because it is already in num, I just wanted the idea to be more discoverable.

1 Like
36

Ha! Sorry, I should have looked back to see who wrote that.

37

Nah youā€™re fine. Itā€™s been 6 months. Both versions were initially authored sometime in February/March.

Time for num to merge: 2 days

Time for rustc to merge: 6 months.

So yeah the crate ecosystem definitely works.

38

Iā€™m not always so fast with num, but yeah, generally crates have fewer constraints and can be more agile.

39

Pinging @aturon. Iā€™m not really sure how we should proceed here and ultimately itā€™s not really my decision to make anyways. You merged the PR originally, should we revert it?

40

@Xaeroxe, I will bring it to the libs team ASAP.

41

The clamp implementation has been reverted by https://github.com/rust-lang/rust/pull/44438 today. What will be necessary steps to bring this back in the future?

  • Make a rule when an inference failure is considered XIB (expected-impl-breakage) (like {f32, f64}::from_bits), and when is it unacceptable (like clamp here).

    Currently type inference change is considered acceptable by RFCs 1105 and 1122 as one could always use UFCS or other ways to force a type. But since the first try of T += &T was closed, clamp is reverted, and given the churn caused by PR #42496 (Ord::{min, max}) there are clearly something that cannot be disregarded as XIB.

  • Whenever we add a method to a std trait, there should be a crater run to ensure the name is not already existing.

  • When downstream crate did not specify #![feature(clamp)], the candidate Ord::clamp should never be considered (a future-compatible warning can still be issued). This will allow introduction of new trait methods not ā€œinsta-breakingā€, but the problem will still come back when stabilizing.

1 Like