Implement DataView: Float16, BigInt64, BigUint64 #2218
Conversation
|
Looks like the right idea to me. Does anyone else want to do a code review first? |
rbri
left a comment
There was a problem hiding this comment.
Not sure if the bit shifting is worth the effort and it might decrease the readability. On the other hand we already do a lot of bit magic in this class ...
Outside of the minor suggestions this is another great step forward, thanks a lot for this PR.
| return Float.valueOf(sign == 0 ? 0.0f : -0.0f); | ||
| } else { | ||
| // Denormalized number | ||
| float value = (float) (mantissa / 1024.0 * Math.pow(2, -14)); |
There was a problem hiding this comment.
maybe we can precalculate some constant value
| if (exponent == 0) { | ||
| if (mantissa == 0) { | ||
| // Zero | ||
| return Float.valueOf(sign == 0 ? 0.0f : -0.0f); |
There was a problem hiding this comment.
i guess the code looks simple if you do not use else after the return (same for the other if else cases)
| } | ||
| } else { | ||
| // Normalized number | ||
| float value = (float) ((1.0 + mantissa / 1024.0) * Math.pow(2, exponent - 15)); |
There was a problem hiding this comment.
this class does a lot of bit shifting, maybe we can should do this for / 1024.0 also?
| // Overflow to infinity | ||
| doWriteInt16(buf, offset, (sign << 15) | 0x7c00, littleEndian); | ||
| return; | ||
| } else if (absVal < (float) Math.pow(2, -14)) { |
There was a problem hiding this comment.
again return/else but you use the other approach in the code above
| } else if (exponent <= 0) { | ||
| // Denormalized | ||
| exponent = 0; | ||
| mantissa = (int) Math.round((absVal / Math.pow(2, -14)) * 1024.0) & 0x3ff; |
There was a problem hiding this comment.
precalculation and bit shift?
|
I'm a little nervous of the bit manipulation here so it would be really helpful to have a written explanation of what's going on. Also @zhurvla it's helpful to write a little bit more than a title in a PR, and it's worth taking care not to accidentally pull in the properties file from our fork, I got a bit of a shock at the amount of change in the first commit until I realised what had happened. |
| } | ||
| } | ||
|
|
||
| public static Float readFloat16(byte[] buf, int offset, boolean littleEndian) { |
There was a problem hiding this comment.
Looking at the other functions in this file... could we implement it in a similar way to the existing readFloat32 and friends?
public static Float readFloat32(byte[] buf, int offset, boolean littleEndian) {
long base = readUint32Primitive(buf, offset, littleEndian);
return Float.valueOf(Float.intBitsToFloat((int) base));
}This looks so much simpler than what you have implemented. Can we do something like it for float 16, relying on readInt16? (Ditto for the setter)
There was a problem hiding this comment.
We don't have a built in function for making single precision float from the bits of a half precision float.
No description provided.