Skip to content

Conversation

@minad
Copy link
Member

@minad minad commented Jul 24, 2019

If we make MP_WARRAY private we do not have to expose limits.h in the tommath.h header. I believe MP_WARRAY is never used by library consumers. We have deprecated it for now, after this change it would be only available in tommath_private.h.

@minad minad requested a review from sjaeckel July 24, 2019 13:57
@minad
Copy link
Member Author

minad commented Jul 24, 2019

Grepping through the debian code base shows that MP_WARRAY appears only in vendored tommath code, but never in library user code.

https://codesearch.debian.net/search?q=MP_WARRAY&perpkg=1

@nijtmans
Copy link
Collaborator

I fully agree! However, this can be improved a little bit:

See:
#define MP_WARRAY (int)(1uLL << (((CHAR_BIT * sizeof(private_mp_word)) - (2 * MP_DIGIT_BIT)) + 1))
It's not really usefull to shift a long long, and finally cast it to an (int).
Better:
#define MP_WARRAY (1 << (((CHAR_BIT * (int)sizeof(private_mp_word)) - (2 * MP_DIGIT_BIT)) + 1))

So: cast the result of the sizeof() operator to (int).

(I know, it already was like that, but if I propose to fix this in another PR then this one has to be rebased anyway)

@minad
Copy link
Member Author

minad commented Aug 1, 2019

I think we also have mp_sizeof_bits. Unfortunately I am on mobile right now. If you have time, @nijtmans feel free to improve and push to this PR. I think you can push directly to this branch instead of opening another PR.

@minad
Copy link
Member Author

minad commented Aug 5, 2019

@nijtmans I guess it is better now?

@minad minad requested a review from nijtmans August 5, 2019 19:13
Copy link
Collaborator

@nijtmans nijtmans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's exactly what I mean!

@sjaeckel
Copy link
Member

sjaeckel commented Aug 6, 2019

I'm not against this change itself, but as it would break our contracts with the user we should push it to a later stage and leave it as deprecated in tommath.h

@minad
Copy link
Member Author

minad commented Aug 9, 2019

@sjaeckel Ok! Any plans on when you want to do the new release (1.2)? After that we could remove the deprecations and continue with 2.0? Would that be ok?

@sjaeckel sjaeckel added this to the v2.0.0 milestone Sep 2, 2019
@minad minad added the finished label Oct 8, 2019
@minad minad force-pushed the do-not-expose-limits-h branch from 1abe6c8 to 5c7947c Compare October 16, 2019 07:13
@minad
Copy link
Member Author

minad commented Oct 16, 2019

close in favor of #378

@minad minad closed this Oct 16, 2019
@sjaeckel sjaeckel removed the finished label Oct 20, 2019
@minad minad deleted the do-not-expose-limits-h branch November 14, 2019 14: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