Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added encoding option to force 32bit encoding for non integer numbers #171

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

janpoltan
Copy link

No description provided.

@hildjj
Copy link
Owner

hildjj commented Aug 19, 2022

I'm assuming I understand your use case, but can you explain it here so we've got it on the record, please?

@hildjj
Copy link
Owner

hildjj commented Aug 19, 2022

Can you please add yourself to the contributors section in package.json?

@janpoltan
Copy link
Author

Added option to force encoding of non integer values to 4 byte float.

@hildjj
Copy link
Owner

hildjj commented Sep 13, 2022

After trying to write tests for this, I'd like to discuss what happens with large floats. They are currently encoded as a 32-bit Infinity (fa 7f 80 00 00), which is different than the 16-bit Infinity that cbor.encode(Infinity) produces (f9 7c 00). I see a couple of options:

  • Document the current behavior.
  • Error.
  • Add support for BigFloat's, which probably breaks your use case and will have interesting interactiosn with the cbor-bigdecimal package, which implements bigfloats.

Of those, document is the easiest of course. Error has precedent, there are several ways to cause an Error to be thrown in Encode - the only niggle is how to detect the issue, but perhaps a couple of checks in _pushFloatBE would work (check the buffer after writing for Infinity and -Infinity). I don't really like the third option.

Regardless, we need to change _pushInfinity, _pushNaN, and _pushIntNum's -0 handling to look at useFloat32bit also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants