Add Chain#knownSize and Chain#lengthCompare (sizeCompare) methods #4159
Conversation
satorg
force-pushed
the
chain-length-compare
branch
from
29d97b6
to
1a9065f
Compare
|
Potentially #4156 can benefit from this one. |
| } | ||
|
|
||
| test("lengthCompare and sizeCompare should be consistent with length and size") { | ||
| forAll { (cu: Chain[Unit], diff: Byte) => |
There was a problem hiding this comment.
I think a more direct law is:
signum(cu.lengthCompare(x)) == signum((cu.length.compareTo(x)))Can we test that one?
| @@ -155,6 +181,34 @@ class ChainSuite extends CatsSuite { | |||
| } | |||
| } | |||
|
|
|||
| test("fromOption should be consistent with one") { | |||
| forAll { (a: Int) => | |||
There was a problem hiding this comment.
I don't see how the value of a matters here. What about a non-property test where you use () as the value.
same comment for below.
satorg
force-pushed
the
chain-length-compare
branch
from
1a9065f
to
a9a094d
Compare
satorg
force-pushed
the
chain-length-compare
branch
from
a9a094d
to
de272da
Compare
| this match { | ||
| case _ if isEmpty => 0 | ||
| case Chain.Singleton(_) => 1 | ||
| case _ => -1 |
There was a problem hiding this comment.
could we add case Chain.Wrap(seq) => seq.knownSize here?
There was a problem hiding this comment.
Yeah, I tried it in the first place, but turned out that 2.12 does not have knownSize implemented. So if we really need it then we'll have to deal with the cross-build compatibility here somehow. Not sure what is the best way to approach that. Any ideas?
There was a problem hiding this comment.
I mean, I can think about two approaches:
- Implement a compat version of
knownSizeforSeqfor 2.12, OR - Handle
case Wrap(seq)in 2.13+ only while keep returning-1in 2.12 unconditionally.
| * res0: Boolean = true | ||
| * }}} | ||
| */ | ||
| final def lengthCompare(len: Long): Int = |
There was a problem hiding this comment.
I wonder about an alternate approach:
// invariant, sz >= 0
final def lengthCompare(len: Int): Int =
this match {
case Empty => Integer.compare(0, len)
case Singleton(_) => Integer.compare(1, len)
case Wrap(seq) => seq.lengthCompare(len)
case _ =>
// length 2 or more... iterate through
}Since this would handle a some common cases without allocation: empty, singletons and chains that are wrapping seqs (which are returned by several operations because they are more efficient).
There was a problem hiding this comment.
Yes sure – we can add more optimizations here. The only note: Chain uses Long for length/size measurements. Do you know why, by chance?
There was a problem hiding this comment.
Added some optimized special cases. I think I may need to add more stress on those cases in tests.
| // `isEmpty` check should be faster than `== Chain.Empty`, | ||
| // but the compiler fails to prove that the match is still exhaustive. | ||
| case _ if isEmpty => 0L.compareTo(len) | ||
| case Chain.Singleton(_) => 1L.compareTo(len) |
There was a problem hiding this comment.
I think this call boxes. I think writing java.lang.Long.compare is safer since we can be 100% sure there is no boxing.
There was a problem hiding this comment.
Yes, it does. Really good catch, thank you. Fixed.
satorg
force-pushed
the
chain-length-compare
branch
from
444df1d
to
3c2d1c3
Compare
|
@johnynek I added even more optimizations to the |
|
Thank you for your keen eye – it helped me to learn some new stuff, I really appreciate it. Cool, I'll let this PR to marinade here for a couple of days before merging in case if someone else would like to take a look at it. |
Adds quite useful
knownSize,lengthCompare(andsizeCompareas an alias) which can be found in Scala Library.