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 cti_bit_length #5

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

Conversation

waywardgeek
Copy link
Contributor

Julien ran into trouble when he needed to encode a big integer as little-endian bytes, since he did not know in advance how big to make the byte buffer. I added cti_bit_length so he could allocate the buffer before doing the conversion.

@waywardgeek
Copy link
Contributor Author

I have to fix this... will upload new version shortly.

@waywardgeek
Copy link
Contributor Author

OK, now it LGTM.

@pornin
Copy link
Owner

pornin commented Sep 4, 2018

I have three comments on that one:

  • There are two notions of bit length that can apply here: one is the bit length of the type (i.e. the maximum size of integers that can be contained), the other is the bit length of the value (which can be shorter). I would prefer to make which one to use explicit in the function name, e.g. cti_bitlength_type() and cti_bitlength_value().

  • If the type bit length is used, then that bit length should not be set to 0 if the value is a NaN; the type for a 75-bit integer is still "a 75-bit integer type" even if the value is currently a NaN. On the other hand, if the value bit length is requested, then returning 0 for a NaN is valid, but then it would require an appropriate value bit length implementation (which can be done in constant-time but requires at least reading all value words).

  • The bit length should be an uint32_t, since there is no guarantee that a size_t would be enough (e.g. on a 16-bit arch, a 70000-bit integer could fit in RAM, but 70000 would not fit in a 16-bit size_t).

I'll fix these in a few days.

@waywardgeek
Copy link
Contributor Author

Thanks!

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