Fix typehint #125
Fix typehint #125
Conversation
Since v1.9.946 ##Inf is supported in Clojurescript
src/loom/alg_generic.cljc
Outdated
| @@ -491,7 +491,8 @@ | |||
| offset (mod idx bits-per-long) | |||
| mask (bit-set 0 offset) | |||
| value (aget new-bitmap chunk) | |||
| new-value (bit-or value ^Long mask)] | |||
| new-value #?(:clj (bit-or value ^Long mask) | |||
There was a problem hiding this comment.
Is this typehint needed?
mask is allocated from bit-set which is a direct call to clojure.lang.Numbers.setBit. This is a static function with a return type of long, so the Clojure compiler already knows that mask is a long value, so no type hinting is needed.
Strangely though, this code typehints to java.lang.Long. The result of this is to box the value up as a java.lang.Long, and call clojure.lang.Numbers.setBit(long,Object). This then passes the second parameter through the clojure.lang.Numbers.bitOpsCast(Object) method, which checks that it's a Long and then calls clojure.lang.RT.longCast(Object). This method checks that it's a Long, and then returns the result of Number.longValue() on it. Which means that it gets all the way back to the original long value.
In other words, all this typehint does is to slow the operation down. It's also worth noting that the above syntax is not allowed at a repl. I recommend getting rid of the hint entirely, which allows the conditional compilation to go away.
There was a problem hiding this comment.
Thank you for your elaborated thoughts :)
I think you are right and I neither see a reason why the typehint was initially added.
My first commit was the minimal change to solve the linked Issue (for clojurescript) but removing the typehint for all targets seems to be the cleaner solution.
|
is there any impediment on merging this @johannesloetzsch @quoll @aysylu ? |
I’m obviously in support of merging this 👍 |
fixes #124