Skip to content

Conversation

@nijtmans
Copy link
Collaborator

Also fix depreciation messages and remove unnecessary includes.

Just some random improvements, not worth to split in 3 different pull requests.

Inspired by #327

@nijtmans nijtmans requested review from minad and sjaeckel August 12, 2019 08:53
@minad
Copy link
Member

minad commented Aug 12, 2019

mp_sizeof_bits cannot be used in tommath.h since it is private

@nijtmans
Copy link
Collaborator Author

mp_sizeof_bits cannot be used in tommath.h since it is private

Indeed. Fixed now.

In addition, I think MP_SIZEOF_BITS() should return an int, not size_t, since it is used to compare against other integers, generally. Follow-up commits demonstrate more simplifications doing that.

@minad
Copy link
Member

minad commented Aug 16, 2019

I think we should rather use size_t at more places instead of int. sizeof_bits should return size_t like sizeof.

@nijtmans
Copy link
Collaborator Author

I think we should rather use size_t at more places instead of int. sizeof_bits should return size_t like sizeof.

If MP_SIZEOF_BITS was a public macro, I would agree with you on that. But MP_SIZEOF_BITS() is used internally, usually comparing with mp_int->used or mp_int->alloc, which are integers. That results in type-casts in the code, that could better be handled in MP_SIZEOF_BITS. This branch is meant to prove that: showing that changing MP_SIZEOF_BITS results in less type-casts in the code. Making it more readable.

But .... if you are fond of type-casts .... ;-)

@sjaeckel
Copy link
Member

sjaeckel commented Sep 9, 2019

... sizeof_bits should return size_t like sizeof.

I think so too

If MP_SIZEOF_BITS was a public macro, I would agree with you on that. But MP_SIZEOF_BITS() is used internally, usually comparing with mp_int->used or mp_int->alloc, which are integers

so instead of making sizeof_bits return the wrong type we will change these types to size_t after 1.2.0 is released. This should make both of you happy and @karel-m unfortunately a bit sadder ... :-\

@nijtmans
Copy link
Collaborator Author

nijtmans commented Sep 9, 2019

If there are plans to change mp_int->used and mp_int->alloc from int to size_t, yes that would make me happy (although it's binary incompatible on 64-bit platforms, so that cannot be done in 1.x)

But - anyway - I'll close this one, if there are such plans, this PR makes no sense.

@nijtmans nijtmans closed this Sep 9, 2019
@sjaeckel sjaeckel deleted the use_sizeof_bits branch September 9, 2019 15:38
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.

4 participants