Resolve boxed math warnings #64
Conversation
valerauko
commented
Nov 8, 2022
- added a bunch of type hints to resolve boxed math warnings
- enabled warnings in the dev profile to motivate people to fix things
| (hash wrapper) | ||
| (hash type))) | ||
| ^long (hash wrapper) | ||
| ^long (hash type))) |
There was a problem hiding this comment.
Hmmm. hash returns an int, not a long. But the compiler uses the hint to force a cast. Not sure which is faster, though.
Before:
public int hashCode() {
return RT.intCast(Numbers.xor(((IFn)const__4.getRawRoot()).invoke(this.wrapper), ((IFn)const__4.getRawRoot()).invoke(this.type)));
}After:
public int hashCode() {
return RT.intCast(RT.uncheckedLongCast((Number)((IFn)const__4.getRawRoot()).invoke(this.wrapper)) ^ RT.uncheckedLongCast((Number)((IFn)const__4.getRawRoot()).invoke(this.type)));
}Truthfully, the best thing is to skip hints here and use explicit conversions to match the types involved. Try:
(hashCode [_]
(int
(bit-xor
(long (hash wrapper))
(long (hash type)))))There was a problem hiding this comment.
Does Clojure even allow ^int type hints? It gives me an "Only long and double primitives are supported" error.
I'll change to explicit cast.
There was a problem hiding this comment.
^int is a valid type hint, but not for function parameters or return values. It's fine in local contexts, like a let.
Hell, it used to be (iirc) that unboxed parameters weren't allowed at all, but it seems like they're allowed for fn arities up to 4 now.
There was a problem hiding this comment.
(You shouldn't need to hint hashCode btw, that's defined by Object.)
| (Conversion. (fn [x options] (s/map #(f % options) x)) (+ cost 0.1))))) | ||
| (Conversion. (fn [x options] (s/map #(f % options) x)) (+ ^double cost 0.1))))) |
There was a problem hiding this comment.
Seems like the hint should be higher up, but it's not working. I forget if this is a limitation of deftype or not.
Probably preferable to convert at the top of the let, like:
(let [cost (double cost)
m' (assoc-in ...There was a problem hiding this comment.
Or a limitation of defprotocol, I mean, which defines IConversionGraph. (Which should be named something else, since it's not an interface.)
There was a problem hiding this comment.
Yeah I tried to add the hint both on defprotocol and deftype but it errored...
There was a problem hiding this comment.
Not surprised. I get the compromises necessary to interop with Java, but the whole deftype/protocol/record system is ugly and sticks out.
|
You know, we already use clj-commons.primitive-math elsewhere, we should probably do the same here for consistency. Sorry, can you change it? |
src/clj_commons/byte_streams.clj
Outdated
| ^ByteBuffer (.limit (int (min lim (+ (int %) ^long chunk-size)))) | ||
| .slice) | ||
| indices)) | ||
| #(-> buf | ||
| .duplicate | ||
| ^ByteBuffer (.position (int %)) | ||
| ^ByteBuffer (.limit (int (min lim (p/+ (int %) chunk-size)))) |
There was a problem hiding this comment.
Hmmm. I wonder why we downcast the index to an int. Since chunk-size is long, the index will be immediately widened back to a long for the addition.
I used m because p was already taken. Should I change the existing alias to something else? |
|
I think |
|
Eastwood fails with the reflection on the (.type dst) which can't be fixed according to the comment |
|
I think this is fine for now. Thanks! Something is still wrong about that def-conversion/AOT problem, but we're not going to fix it here. It seemed related to CLJ-1650/1741, but the timing is wrong; that bug was fixed in Clojure 1.8. I wonder if it's related to Potemkin and |
|
Thanks! I guess the warnings in manifold are next! |