add identity hash to alleycats #3944
Conversation
There was a problem hiding this comment.
Just saw this, nice addition and clean-up on my PR! I don't think anything is supposed to go in the .jvm folder. The correct way to handle this is to change the build to a full cross with a proper jvm folder. Is System.identityHash not available on Scala.js and/or Native?
Also in your clean up efforts you removed ReferentialEqTests, I'm surprised the test suite passed (must be chance?) since it actually doesn't satisfy all the Eq laws.
|
|
||
| import cats.Hash | ||
|
|
||
| class SystemIdentityHash[A <: AnyRef] extends ReferentialEq[A] with Hash[A] { |
There was a problem hiding this comment.
Maybe just me, but perhaps make it a trait so that it can be mixed with other instances?
There was a problem hiding this comment.
I don't mind either way. I'll modify the change
alleycats-tests/shared/src/test/scala/alleycats/tests/ReferentialEqSuite.scala
Show resolved
Hide resolved
|
I checked and |
alleycats-tests/shared/src/test/scala/alleycats/tests/AlleycatsSuite.scala
Outdated
Show resolved
Hide resolved
SimY4
force-pushed
the
topic/identity-hash
branch
2 times, most recently
from
34fdf93
to
c3e35de
Compare
SimY4
force-pushed
the
topic/identity-hash
branch
from
c3e35de
to
3ebe12c
Compare
SimY4
force-pushed
the
topic/identity-hash
branch
from
2b89e76
to
f54101c
Compare
|
The thing that I haven't implemented but could be potential optimisation is: merge both ReferentialEq and SystemIdentityHash into one alleycats definition since they are very closely related. |
I think it's fine to have them as separate traits, more modular :) As it was pointed out in the original PR #3925 (comment) using typeclasses isn't particularly performant anyway! |
This is a follow-up PR based on the discussion in #3925
To compliment ReferentialEq I've added SystemIdentityHash to JVM alleycats.