Adds Support for 32-bit Floats #84
Conversation
| buffer.writeUInt8(FLOAT_TYPE); | ||
| buffer.writeUInt64(doubleToRawLongBits(value)); | ||
|
|
||
| if (value == ((double) ((float) value))) { |
There was a problem hiding this comment.
Minor nit, the code does have a legacy style convention that we should follow (braces on its own line).
There was a problem hiding this comment.
It might be a good idea to make this behavior configurable. binary32 support was added as a relatively late feature in Ion and to maximize compatibility with other clients we should make this an opt-in.
The other ancillary benefit to opt-in is avoiding the narrow/widening/comparison machinery needed to do this which could have (probably marginal) performance implication.
|
Overall, this is an excellent fix--thanks for working on it! A couple top-level comments. We should make the writer support opt-in for the auto-sizing capability. This is mostly for compatibility (this was one of the latent adds to the specification before release). The best way to do this is probably through the builder interface in I've added amazon-ion/ion-tests#11 separately track adding this to the test vector suite. |
IonReaderBinaryRawX has learned to read the length of the float type. Ref: http://amznlabs.github.io/ion-docs/float.html
IonBinarWriterBuilder gained several methods for configuring Binary32 support. When enabled IonBinaryWriters conditionally write floats as Binary32 when there will be no loss in precision. Ref: http://amznlabs.github.io/ion-docs/float.html
hohle
force-pushed
the
32-bit-floats
branch
2 times, most recently
from
4e109bd
to
000a83a
Compare
|
Broke the PR up into two commits; the first adds support for reading 32-bit floats, the second adds support for conditionally writing them based on configuration and precision. |
|
Almann, I don't understand why we'd opt-in this feature. What do we need to be compatible with? This library has no users of older software. |
|
@toddjonker - I'm being conservative about what software this library may interact with. If you think I'm being too conservative, then I am happy to consider changing things--but having a switch to control this serialization seems like an easy way to make that a non-issue. |
|
Thanks again @hohle! My apologies for taking a bit to merge this in. |
IonReaderBinaryRawX has learned to read the length of the float
type.
IonRawBinaryWriter has learned how to write 32-bit float values
when there will be no loss in precision.
Ref: http://amznlabs.github.io/ion-docs/float.html