Skip to content

SI-10086 NumericRange.min|max with custom Integral#5575

Merged
lrytz merged 2 commits intoscala:2.11.xfrom
gzm0:fix-numeric-range-min
Dec 12, 2016
Merged

SI-10086 NumericRange.min|max with custom Integral#5575
lrytz merged 2 commits intoscala:2.11.xfrom
gzm0:fix-numeric-range-min

Conversation

@gzm0
Copy link
Contributor

@gzm0 gzm0 commented Dec 2, 2016

No description provided.

@som-snytt
Copy link
Contributor

I didn't see this. You can copy the unit test. It passed locally.

https://github.com/scala/scala/compare/2.12.x...som-snytt:issue/10086-max-order?expand=1

@som-snytt
Copy link
Contributor

It's not obvious to me that the following min should be slow-path. If the ordering passed to min/max is the constructor Integral, ord eq num, can it just return start/last. (I noticed Numeric was not always an Ordering.)

scala> import scala.collection.immutable.NumericRange, math._
import scala.collection.immutable.NumericRange
import math._

scala> val sz = 1000000000
sz: Int = 1000000000

scala> implicit val customIntegral = new Numeric.IntIsIntegral with Ordering.IntOrdering { }
customIntegral: scala.math.Numeric.IntIsIntegral with scala.math.Ordering.IntOrdering = $anon$1@127a7272

scala> val nr = NumericRange(1, sz, 1)
nr: collection.immutable.NumericRange.Exclusive[Int] = NumericRange 1 until 1000000000

scala> nr.min
res0: Int = 1

@gzm0 gzm0 force-pushed the fix-numeric-range-min branch from 946e9cc to 8800717 Compare December 3, 2016 19:00
@gzm0 gzm0 changed the title WIP Fix SI-10086 SI-10086 NumericRange.min|max with custom Integral Dec 3, 2016
@gzm0 gzm0 force-pushed the fix-numeric-range-min branch 2 times, most recently from a9a4523 to ca016a4 Compare December 3, 2016 21:00
@gzm0 gzm0 force-pushed the fix-numeric-range-min branch from ca016a4 to de0a1fa Compare December 3, 2016 21:53
@gzm0
Copy link
Contributor Author

gzm0 commented Dec 4, 2016

This is ready for review.

@som-snytt
Copy link
Contributor

Probably review by @Ichoran "Ichorange".

case class A(v: Int)

implicit object aIsIntegral extends scala.math.Integral[A] {
def compare(x: A, y: A): Int = x.v - y.v
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inconsistent. Orderings must be transitive with respect to the sign of compare but this is not true with A(Int.MinValue), A(0), and A(Int.MaxValue). Though extreme values are not used in this test, it is nonobvious that this method fails, and there is no advantage to using subtraction rather than the compare method on the underlying Ints.

@Ichoran
Copy link
Contributor

Ichoran commented Dec 5, 2016

It would be good to have something--at least a commit message--that explains why (ord eq num) could be true. It's not utterly obvious.

@gzm0
Copy link
Contributor Author

gzm0 commented Dec 7, 2016

Updated. Please let me know when you would like me to squash the commits.

@lrytz
Copy link
Member

lrytz commented Dec 12, 2016

LGTM, squashing on merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants