Add {C,c}omparison to Order, fixes #1101 #1102
Conversation
adelbertc
changed the title
| def comparison(x: A, y: A): Comparison = { | ||
| val r = compare(x, y) | ||
| if (r > 0) Comparison.GreaterThan | ||
| else if (r == 0) Comparison.EqualTo |
There was a problem hiding this comment.
Is it OK we're using == instead of Eq[Int] ? I figure it's more performant but maybe not?
There was a problem hiding this comment.
Yes, when working with a known primitive (e.g. Int) using == is correct.
There was a problem hiding this comment.
I agree with @ceedubs that this should probably use Comparison.fromInt(compare(x, y)) (and that the code here should be moved to Comparison).
|
If we're going to do this, we should also create a |
|
@non would it make sense to have a |
|
Yeah, returning |
|
I like |
| case Comparison.GreaterThan => 1 | ||
| case Comparison.EqualTo => 0 | ||
| case Comparison.LessThan => -1 | ||
| } |
There was a problem hiding this comment.
out of curiosity, @non, do you know it the above is faster or:
sealed abstract class Comparison(val toInt)
case object GreaterThan extends Comparison(1)
case object EqualTo extends Comparison(0)
case object LessThan extends Comparison(-1)It seems to me the above would be.
There was a problem hiding this comment.
Yeah I think having Comparison instances "know" their integer value (either through fields or through methods on the subtype) would be better than using pattern matching in .toInt.
There was a problem hiding this comment.
It may be different with a val like that, but fwiw, at one point I benchmarked having scalaz's Maybe implement fold via pattern-matching in the parent vs dynamic dispatch to the child implementation and the former seemed to have a slight edge. It may be worth throwing together a benchmark if this is something that we care about. (I'm okay with it not being something that we care about, since people can always use the Int version of compare if they really want performance.)
Current coverage is 88.62%
@@ master #1102 diff @@
==========================================
Files 226 227 +1
Lines 2917 2944 +27
Methods 2867 2884 +17
Messages 0 0
Branches 48 57 +9
==========================================
+ Hits 2594 2609 +15
- Misses 323 335 +12
Partials 0 0
|
|
Looks great! Thanks so much. 👍 |
|
Nice! Thanks @adelbertc. |
|
@ceedubs is that a +1 ? |
|
@non yep 👍 when the build succeeds. |
Review by @non and @johnynek