[WIP, RFC] introduce various mp_set_i and mp_set_u functions with precise types

[minad]

Request for comments/feedback: This is a proposal for set_int/set_long overhaul using precise types. The downside is that the current getter/setter are deprecated which will probably affect all users. I don't see a better alternative however since the current function are flawed in some sense:

• mp_err return value for non failing setters (mp_set_int), should be void
• setters take unsigned values, no possibility to set signed values directly
• getters return unsigned values, no possibility to get signed values directly
• no support for precise types

Note that there is a desire for precise types for example in language implementations which precisely need to know at which size to switch from boxed to unboxed biginteger representations.

See e.g. MVM_bigint_mp_set_uint64 from MoarVM https://github.com/MoarVM/MoarVM/blob/2fe2f588fd931e9bed982da97f352207ddab9e62/src/math/bigintops.c#L30

Note that this is not yet a finished PR, I basically only modified tommath.h here.

[minad]

Another interesting observation - we introduced MP_WUR in develop.h. I've seen this will lead to warnings on many tommath calls in MoarVM. They are probably using an allocator which never fails (I haven't checked that). Even if they do, they could still miss logical errors. (These warnings can be disabled by defining MP_WUR).

But what I want to say - users upgrading to a newer tommath will already get warnings. So maybe deprecating more things will make things only slightly more annoying for users, but in the end things will be better and safer.

[sjaeckel]

Note that this is not yet a finished PR, I basically only modified tommath.h here.

that explains the missing implementation I was searching for in the diff :D

The downside is that the current getter/setter are deprecated which will probably affect all users.

could we probably provide some scripts to move code-bases to the new API?

but in the end things will be better and safer.

I think so too, it feels like evolving in a good direction

[minad]

that explains the missing implementation I was searching for in the diff :D

I wanted to gather a bit more feedback before doing the work ;)

could we probably provide some scripts to move code-bases to the new API?

I don't think a script will be useful. It is impossible to make it such that it is safe in many cases and won't require manual intervention. For example the new setters return void. If you have a codebase checking the error return values, the script must recognize the conditional etc etc.
Furthermore I think many codebases using the lib are some kind of bindings (scripting languages, language runtimes etc), which call e.g., mp_set_long maybe once. Also note that we have precise deprecation messages which will tell you what to use instead.

[sjaeckel]

TBH I'm tempted to skip the int/long variants and let the user do that if it's needed

not sure though if I'm missing something there why we should provide them...

[minad]

Well this is some kind of philosophical question. These days I mostly write code with precise types int32_t. The only imprecise types I am using are intptr_t, size_t etc. This way I make it very clear if a value fits inside a variable. However I am also usually write code only for 32 and 64 bit architectures and want to stay portable across those. Usually they are also int=int32_t always. Performance wise int32 vs int64 does not make a difference on the 64 bit archs. I only have to use int64 when I really need it then.

If I want to code against architectures with int=int16_t or stay portable across 16/32/64 bit, one has to probably use imprecise types like int and long in order to use the most efficient int type for loops etc. However one takes the overflow risk then.

And there are probably many people who don't want the precise types. For those people I added this API, but it is also somehow low cost, since it is just a macro which selects the underlying function or in the i32 case, just an alias. Also note that tommath takes the imprecise stand as of now. We are not using precise types internally for example. This becomes a hypothetical problem with numbers larger than 2^(16*65535) or so ;)

[minad]

In other words - I've read the following statements.

1. "Use stdint.h types to stay portable"
2. "Use int/long types to stay portable"

🙈

[minad]

Ping @sjaeckel I worked a bit on that and this is how things look now. Quite a lot. Am I overcomplicating things? I am distinguishing between mag32 and u32 (two complement) for example. However some things are just macros.

But I am also deprecating 9 functions. Introducing 12 new ones + many macros.

/* get integer, set integer and init with integer (int32_t) */
int32_t mp_get_i32(const mp_int *a) MP_WUR;
void mp_set_i32(mp_int *a, int32_t b);
mp_err mp_init_i32(mp_int *a, int32_t b) MP_WUR;

/* get integer, set integer and init with integer (int64_t) */
int64_t mp_get_i64(const mp_int *a) MP_WUR;
void mp_set_i64(mp_int *a, int64_t b);
mp_err mp_init_i64(mp_int *a, int64_t b) MP_WUR;

/* get integer, set integer and init with integer, behaves like two complement for negative numbers (uint32_t) */
#define mp_get_u32(a) ((uint32_t)mp_get_i32(a))
void mp_set_u32(mp_int *a, uint32_t b);
mp_err mp_init_u32(mp_int *a, uint32_t b) MP_WUR;

/* get integer, set integer and init with integer, behaves like two complement for negative numbers (uint64_t) */
#define mp_get_u64(a) ((uint64_t)mp_get_i64(a))
void mp_set_u64(mp_int *a, uint64_t b);
mp_err mp_init_u64(mp_int *a, uint64_t b) MP_WUR;

/* get magnitude */
uint32_t mp_get_mag32(const mp_int *a) MP_WUR;
uint64_t mp_get_mag64(const mp_int *a) MP_WUR;

/* get integer, set integer and init with integer (int) */
#define mp_get_i(a)        ((int)mp_get_i32(a))
#define mp_set_i(a, b)     mp_set_i32((a), (b))
#define mp_init_i(a, b)    mp_init_set_i32((a), (b))

/* get integer, set integer (unsigned int) */
#define mp_get_u(a)        ((unsigned)mp_get_u32(a))
#define mp_set_u(a, b)     mp_set_u32((a), (b))
#define mp_init_u(a, b)    mp_init_u32((a), (b))

/* get integer, set integer (long) */
#define mp_get_l(a)        (sizeof (long) == sizeof (int64_t) ? (long)mp_get_i64(a) : (long)mp_get_i32(a))
#define mp_set_l(a, b)     (sizeof (long) == sizeof (int64_t) ? mp_set_i64((a), (b)) : mp_set_i32((a), (int32_t)(b)))
#define mp_init_l(a, b)    (sizeof (long) == sizeof (int64_t) ? mp_init_i64((a), (b)) : mp_init_i32((a), (int32_t)(b)))

/* get integer, set integer (unsigned long) */
#define mp_get_ul(a)       (sizeof (long) == sizeof (int64_t) ? (unsigned long)mp_get_u64(a) : (unsigned long)mp_get_u32(a))
#define mp_set_ul(a, b)    (sizeof (long) == sizeof (int64_t) ? mp_set_u64((a), (b)) : mp_set_u32((a), (uint32_t)(b)))
#define mp_init_ul(a, b) (sizeof (long) == sizeof (int64_t) ? mp_init_u64((a), (b)) : mp_init_u32((a), (uint32_t)(b)))

[minad]

Hmm. We could also get rid of the imprecise functions and make the API a bit less verbose by giving priority to the 32 bit functions. Only the 64 bit functions get a suffix then.

/* get integer, set integer and init with integer (int32_t) */
int32_t mp_get_i(const mp_int *a) MP_WUR;
void mp_set_i(mp_int *a, int32_t b);
mp_err mp_init_i(mp_int *a, int32_t b) MP_WUR;

/* get integer, set integer and init with integer, behaves like two complement for negative numbers (uint32_t) */
#define mp_get_u(a) ((uint32_t)mp_get_i(a))
void mp_set_u(mp_int *a, uint32_t b);
mp_err mp_init_u(mp_int *a, uint32_t b) MP_WUR;

/* get integer, set integer (int64_t) */
int64_t mp_get_i64(const mp_int *a) MP_WUR;
void mp_set_i64(mp_int *a, int64_t b);

/* get integer, set integer, behaves like two complement for negative numbers (uint64_t) */
#define mp_get_u64(a) ((uint64_t)mp_get_i64(a))
void mp_set_u64(mp_int *a, uint64_t b);

/* get magnitude */
uint32_t mp_get_mag(const mp_int *a) MP_WUR;
uint64_t mp_get_mag64(const mp_int *a) MP_WUR;

I think this is a better solution. 9 functions deprecated/replaced, 10 functions + 2 macros added.

[minad]

Now that I am trying to convert the testsuite to the reduce API, I am unconvinced again. The test suite never uses precise types and things get ugly with the precise API. I can imagine users having the same issues. It is simply not a good idea to force users to change all their types.

They should somehow have the option and we should only give the guarantee (in contrast to now!) that the full width of the type is respected, meaning set/get_int/long/... should round trip.

I think there is not really a way around the full thing (i32, u32, i64, u64, int, long)...

[minad]

closed in favor of #285