Add Order instance for scala.Symbol #1395
Conversation
Current coverage is 91.68% (diff: 100%)@@ master #1395 diff @@
==========================================
Files 239 240 +1
Lines 3607 3610 +3
Methods 3544 3546 +2
Messages 0 0
Branches 62 63 +1
==========================================
+ Hits 3307 3310 +3
Misses 300 300
Partials 0 0
|
|
|
||
| class SymbolOrder extends Order[Symbol] { | ||
| override def eqv(x: Symbol, y: Symbol): Boolean = | ||
| x eq y |
There was a problem hiding this comment.
Any reason not to use == here? I assume Symbol's equal method is correct and we don't need to worry here that eq is correct.
There was a problem hiding this comment.
Symbols are interned ... might as well take advantage of that fact.
There was a problem hiding this comment.
Yes, that was my intention.
There was a problem hiding this comment.
yes, I know that eq is how it is implemented due to the interning, but equals already does this:
https://github.com/scala/scala/blob/2.12.x/src/library/scala/Symbol.scala#L31
so, this, to me, assumes the jit may not do the job, yet each reviewer of the code has to recall this fact about how Symbol instances are created.
That said, I'm happy to simply add a comment here to explain this so that the code is clearer to someone unfamiliar (I think Symbol is actually rather unused as a scala feature these days).
There was a problem hiding this comment.
Ok, comment added in 318e456.
|
👍 |
1 similar comment
|
👍 |
No description provided.