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

min, max, and clamp apply to all Comparables #3176

Closed
DartBot opened this issue May 22, 2012 · 8 comments
Closed

min, max, and clamp apply to all Comparables #3176

DartBot opened this issue May 22, 2012 · 8 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-obsolete Closed as the reported issue is no longer relevant core-2 P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@DartBot
Copy link

DartBot commented May 22, 2012

This issue was originally filed by @seaneagan


dart:math (formerly the Math class) has min and max methods which accept and return nums, issue #44 additionally requests a related "clamp" method. These methods should be generalized to accept and return any Comparables.

For such methods to be type safe, they should be generic (issue #254 - Milestone-Later, which would cause type warnings (checked mode errors)
if these methods were made generic after the fact), and Comparable should have a type parameter(issue #1492). They would look something like:

T min<T extends Comparable<T>>(T x, T y);
T max<T extends Comparable<T>>(T x, T y);
T clamp<T extends Comparable<T>>(T x, T min, T max);

Generic method support would not be required though if they were instead instance methods in Comparable (which would also avoid the type argument on calls).

They could also potentially be static methods on Comparable if statics were supported in interfaces (http://dartbug.com/2975 except without restriction to go through a
default class).

@kasperl
Copy link

kasperl commented May 24, 2012

Added Area-Library, Triaged labels.

@lrhn
Copy link
Member

lrhn commented Aug 19, 2013

Generic Comparable versions of clamp, min and max would be fine, but they would not give the same results as dart:math's num versions (even if the only difference is on NaN).

@lrhn
Copy link
Member

lrhn commented Aug 19, 2013

Removed Type-Defect label.
Added Type-Enhancement label.

@DartBot
Copy link
Author

DartBot commented Oct 13, 2014

This comment was originally written by @seaneagan


min, max, and clamp (and many others) are available as top-level functions in the contrast package:

https://pub.dartlang.org/packages/contrast
https://github.com/seaneagan/contrast

Of course if they were instance methods then they would propogate type info better. I think the difference with NaN is actually a good reason to make them instance methods (polymorphism), would just need to be documented.

@DartBot DartBot added Type-Enhancement area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. labels Oct 13, 2014
@kevmoo kevmoo added P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug and removed triaged labels Feb 29, 2016
@lrhn lrhn added the core-m label Aug 11, 2017
@floitschG floitschG added core-2 and removed core-m labels Aug 31, 2017
@matanlurey matanlurey added the closed-obsolete Closed as the reported issue is no longer relevant label Jun 22, 2018
@matanlurey
Copy link
Contributor

I imagine the cost of adding these to Comparable without either default functions or new language features that allow incrementally adding APIs (extension methods) is not feasible without lots of effort.

copybara-service bot pushed a commit that referenced this issue Oct 15, 2021
The pub client still won't send analytics with this PR. (This will be
enabled in a follow-up).

Changes:
```
> git log --format="%C(auto) %h %s" 0764437..35681b0
 https://dart.googlesource.com/pub.git/+/35681b01 Analytics (#2778)
 https://dart.googlesource.com/pub.git/+/9c65d31c Accept 'platforms' property in pubspec.yaml (#3176)
 https://dart.googlesource.com/pub.git/+/1af0de54 Accept 'screenshots' property in pubspec.yaml (#3174)
 https://dart.googlesource.com/pub.git/+/19045b95 Tests related to token authentication (#3147)
 https://dart.googlesource.com/pub.git/+/f36da07f Use applicationConfigHome from package:cli_util (#3164)

```

Diff: https://dart.googlesource.com/pub.git/+/0764437088fd58eb7af779ecef66bab40dfcf2e9~..35681b0126a1fb48bf2062dd09f74296715402c2/
Change-Id: I7313f3125934aba3473b2725074bfc7fd92e25e4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/217004
Reviewed-by: Jonas Jensen <[email protected]>
Commit-Queue: Sigurd Meldgaard <[email protected]>
@cbatson
Copy link

cbatson commented Dec 12, 2022

It's unclear why this was closed. Can it be re-opened? (Especially given the advent of extensions.)

@rakudrama
Copy link
Member

@cbaston I think a reason (mentioned in 2013) is that min written purely in terms of compareTo gives the wrong result for double.nan.

@lrhn
Copy link
Member

lrhn commented Feb 27, 2023

The min and max from dart:math preserve NaN, but num.clamp is actually written to be compatible with num.compareTo, treating NaN as the largest value.
It's also the one which is an instance method, so we can't just generalize it to other types.

We could add static min, max and extension clamp methods to Comparable, like:

class Comparable<T> { 
  // ...
  static T min<T extends Comparable<T>>(T v1, T v2) => v1.compareTo(v2) <= 0 ? v1 : v2;
  static T max<T extends Comparable<T>>(T v1, T v2) => v1.compareTo(v2) >= 0 ? v1 : v2;
}
extension ClampComparable<T extends Comparable<T>> on T {
  T clamp(T min, T max) => compareTo(min) < 0 ? min : compareTo(max) > 0 ? max : this;
}

The clamp would not apply to num, since it already has a clamp method.
(It also wouldn't match int or double, since they don't implement Comparable<int> or Comparable<double> respectively, they implement Comparable<num>, and type inference won't guess that num is the solution it needs here.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-obsolete Closed as the reported issue is no longer relevant core-2 P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

8 participants