Various deprecations

[minad]

• mp_jacobi in favor of mp_kronecker (has already been somehow deprecated it seems)
• mp_get_bit because of mixed up return type
• move some functions to bn_deprecated.c

Note: This is based on #258

[nijtmans]

• mp_get_bit because of mixed up return type

I don't understand this: So you don't agree that this function can return both a boolean and MP_VAL?

What's the replacement?

[minad]

I don't understand this: So you don't agree that this function can return both a boolean and MP_VAL?

If you want to use precise types, this is not possible. The return value is basically a union of mp_bool and mp_err. Therefore the return type is just an int. This is the only function which mixes this up.

Better signatures would be:

mp_bool mp_get_bit(const mp_int*, unsigned int);
mp_err mp_get_bit(const mp_int*, int, mp_bool*);

What's the replacement?

The replacement is mp_bool s_mp_get_bit(const mp_int*, unsigned int). It is in tommath_private.h now, but we can also make it public under a different name if we need it. Do we need it to be public?
I don't think mp_ints are used as bitsets as of now since there is no mp_set_bit available.

[nijtmans]

I would be OK with returning MP_NO in case of error in stead of MP_VAL, and making the return-type mp_bool: The only situation this happens is when b < 0, clearly a programming error. But ... it's not a big deal. The situation b < 0 could be interpreted as requesting fractional bits, but ... ltm only handles integers in which all fractional bits are 0!!!

I'm using mp_get_bit(0) as a kind of replacement for mp_isodd(), in order to arrange for binary upwards/downwards compatibilith between ltm 1.1.0 and (upcoming) 1.2.0.

[minad]

@nijtmans I see that you need the function for now. Even if we deprecate the function it would still be available for some time allowing you to stay compatible. At some point in the future the deprecated functions would go away. But when this is going to happen is undecided.

My point about deprecating mp_get_bit was also motivated by the fact that the function somehow stands out (since there is no mp_set_bit, see also #243). As a way forward, a good alternative would probably be introducing new functions mp_bit_get and mp_bit_set. This would at least make me most happy since the API would be coherent and there would be a precise return type. And what @czurnieden suggested, maybe also add mp_bit_toggle.

mp_bool mp_bit_get(const mp_int*, size_t); /* size_t or unsigned int */
mp_err mp_bit_set(const mp_int*, size_t, mp_bool);
mp_err mp_bit_toggle(const mp_int*, size_t);

About your suggestion of changing semantics - this is also a possibility and in this case probably no big deal since MP_VAL is clearly a programming error and a very obvious one. But I think the proper way to do things is just introducing new functions and slowly getting rid of the deprecated ones. In this case changing the index to size_t or unsigned fixes the problem completely.

[nijtmans]

In addition, shouldn't the bit-functions us two's-complement? But ... that's out-of-scope for this PR, so let's continue with this. OK!

[nijtmans]

I'm OK with this change. Replacement set-functions can be handled in a separate PR

[minad]

@nijtmans Ok. I agree that we should do the bit functions separately. I guess you are also ok with #258?

[nijtmans]

Thinking more about it: How about defining now:

mp_bool mp_bit_get(const mp_int*, unsigned int);

This is fully upwards compatible with what it is now, no need to deprecate mp_bit_get(), and no possibility of errors any more.

[czurnieden]

(I'm too slow, this has been written before your comment)

I don't think mp_ints are used as bitsets as of now since there is no mp_set_bit available.

The mp_set_bit is used for numerical purposes in LTM (in computing the Lucas sequence. ) It is also used in computing large factorials (not in LTM), can be used as a sliding window of size one (e.g.: for exponentiation with MP_8BIT, so you don't need to shift the whole exponent if it is a bigint with more than one limb. Not in LTM and not in the all-bigint version I offered.) and probably in a bunch of other algorithms.

Or shorter: it stands on its own, especially in LTM which leans more in the direction of number theory.
mp_set_bit and mp_toggle_bit needed to implement a bitset don't have much use outside that bitset. But as said above: it is no problem for me to implement the missing two. My offer still stands, just say something.

In addition, shouldn't the bit-functions us two's-complement?

Why?

[minad]

Thinking more about it: How about defining now: mp_bool mp_bit_get(const mp_int*, unsigned int); This is fully upwards compatible with what it is now, no need to deprecate mp_bit_get(), and no possibility of errors any more.

If we do that we break the API in a strict sense (disallowing implicit int/uint conversions). Furthermore negative indices return MP_VAL now, and would then return MP_NO.

[minad]

The mp_set_bit is used for numerical purposes in LTM (in computing the Lucas sequence. ) It is also used in computing large factorials (not in LTM), can be used as a sliding window of size one (e.g.: for exponentiation with MP_8BIT, so you don't need to shift the whole exponent if it is a bigint with more than one limb. Not in LTM and not in the all-bigint version I offered.) and probably in a bunch of other algorithms.
Or shorter: it stands on its own, especially in LTM which leans more in the direction of number theory.
mp_set_bit and mp_toggle_bit needed to implement a bitset don't have much use outside that bitset. But as said above: it is no problem for me to implement the missing two. My offer still stands, just say something.

@czurnieden Thanks for chiming in here. The question here is if the functions must be exposed in the public API or if they are more internal? But independent on how we decide on that question - if we want to make the types precise (and not just returning the mixed error/bool int), we need to go via a new replacement function and deprecation. Right now the function is left imprecise in #258.

[minad]

@sjaeckel I prematurely linearized my PRs on top of fperrad's linting PR for your consumption #259 -> #258 -> #260 😀

[minad]

@sjaeckel rebased on top of develop

[minad]

@sjaeckel This is ready from my side. Added enum fixes and rebased.