deprecate mp_expt_d and mp_n_root in favor of mp_expt and mp_root #304

minad · 2019-05-29T18:39:03Z

This PR removes the dependency on mp_digit for mp_expt, mp_n_root and mp_ilogb. It is advantageous to use uint32_t for the base/exponent. This has been partially discussed before in API discussions/overhaul #243.
See also Alternative algorithm in mp_n_root_ex #202 for a better mp_root algorithm which is ready (or ready soon)
mp_ilogb prototype can be changed without deprecation since it hasn't been released yet
@czurnieden Given this PR I guess we can remove the 8 bit special casing in mp_root?
mp_expt_u32 or mp_root_u32 would also be possible names, but I don't think we will offer multiple variants (this is different from mp_add, where we have the specialised mp_add_d and might also offer specialised mp_add_i32/u32).

czurnieden

See also #202 for a better mp_root algorithm which is ready (or ready soon)

It could be ready when you stop changing things for a second so I can rebase&adapt without the risk of being out-of-date when done just 5 minutes later! ;-)

Given this PR I guess we can remove the 8 bit special casing in mp_root?

That ugly thing? Happy when it's gone.

Renaming mp_n_root to mp_root?
There is no real need for that but if you think so.

and removed request for czurnieden

That didn't work. >;->

minad · 2019-05-29T22:14:39Z

It could be ready when you stop changing things for a second so I can rebase&adapt without the risk of being out-of-date when done just 5 minutes later! ;-)

Sorry, I seriously don't want to sabotage your stuff ;)

That ugly thing? Happy when it's gone.

👍

Renaming mp_n_root to mp_root? There is no real need for that but if you think so.

I did that for API/ABI compatibility. Introducing a new name, deprecating the old.

and removed request for czurnieden

That didn't work. >;->

Ouch 😬

sjaeckel · 2019-05-31T08:18:14Z

tommath.h

+mp_err mp_ilogb(const mp_int *a, uint32_t base, mp_int *c) MP_WUR;
+
+/* c = a**b */
+mp_err mp_expt(const mp_int *a, uint32_t b, mp_int *c) MP_WUR;


TBH API consistency-wise I would prefer

mp_err mp_expt_u32(const mp_int *a, uint32_t b, mp_int *c) MP_WUR; mp_err mp_expt(const mp_int *a, const mp_int *b, mp_int *c) MP_WUR;

where mp_expt() returns MP_VAL for b > u32_max and then calls mp_expt_u32()

this would also close #240

#240 also proposes to handle 0, +-1 base special cases for arbitrary large exponent. But I am not convinced that I want that.

If we don't add a all bigint mp_expt I am against the suffix since there are no variants of the function.

I don't see a reason not to do it if it helps the user write cleaner code... that's the purpose of a library... isn't it?

#240 also proposes to handle 0, +-1 base special cases for arbitrary large exponent.

that's a good question if we should start adding special cases... if they're not a lot of effort to implement and maintain, why not? that's just another feature and we provide a lot of features which were a lot of effort

But I am not convinced that I want that.

Why?

If we don't add a all bigint mp_expt I am against the suffix since there are no variants of the function.

that's why I want the all-bigint version now :-) (also a user could then provide its own arbitrary-size mp_expt() implementation e.g. the one from #201 if he really sees the need for it)
also it's consistent and I like consistency

I don't think mp_expt is a good function since it will fail on most inputs and it will only work on very specific inputs.

will work for <= UINT32_MAX (or even << UINT32_MAX for large bases)

won't work for > UINT32_MAX, except for the base is 0,+-1

This kind of special handling is better done by the user. If the user wants it, it is very easy to implement by extracting the low bits as discussed in #240.

that's why I want the all-bigint version now :-)

If you want it feel free to add it - I won't complain too much ;) But I think the API proposed in this PR is the most reasonable one. I guess there are more functions which you could generalise (in a bad way) by promoting arguments from int to mp_int. Do you want these too?

To say it in different words - the type of the function mp_expt(const mp_int*, const mp_int*, mp_int*) makes a promise that it works on more inputs than it does really or in practice. Therefore I don't like it.

I guess there are more functions which you could generalise (in a bad way) by promoting arguments from int to mp_int. Do you want these too?

good point! now I'm not so sure anymore

hmm ...I just scrolled through the API and there are not many math operations which could be promoted ... and they're basically all the exact same case as for this discussion... (I probably missed some operations!?)

/* c = a / 2**b, implemented as c = a >> b */ mp_err mp_div_2d(const mp_int *a, int b, mp_int *c, mp_int *d) MP_WUR; /* c = a * 2**b, implemented as c = a << b */ mp_err mp_mul_2d(const mp_int *a, int b, mp_int *c) MP_WUR; /* c = a mod 2**b */ mp_err mp_mod_2d(const mp_int *a, int b, mp_int *c) MP_WUR; /* computes a = 2**b */ mp_err mp_2expt(mp_int *a, int b) MP_WUR;

Yes and we should neither promote those nor should we promote mp_expt.

tommath.h

minad · 2019-06-04T09:35:31Z

@sjaeckel Ready?

sjaeckel · 2019-06-07T06:36:29Z

@sjaeckel Ready?

as you probably already realized, I'm undecided...

minad · 2019-06-07T08:37:28Z

as you probably already realized, I'm undecided...

Any idea on how to move this forward? As I see it, this PR is relatively benign, replacing mp_digit with a precise type, which is stable over platform changes. This fits the API changes we made before. Further additions (e.g. of bigint only functions) can also be made in separate PRs on a per case basis. Basically the only thing which needs to be decided is the naming. I went for the most direct, least ugliest naming here. This might lead to clashes/naming inconsistency however if you would want to add variants. But as I see it, no variants makes sense. For example as I argued before, mp_expt_bigint is a very partial/sparse function, which would only work for very few inputs and is imo not such a good idea for that reason.

nijtmans · 2019-06-13T13:58:29Z

Continuing the discussion from #321, which actually belongs here ....

@minad wrote:

I would also like to avoid things like

#ifdef no_stdint
mp_err mp_expt(const mp_int*, unsigned);
#else
mp_err mp_expt(const mp_int*, uint32_t);
#endif

Me too. There should only be a single function like:
mp_err mp_expt(const mp_int*, sometype);

I let @sjaeckel and @minad (or whoever else is involded) decide what "sometype" should be. Most obvious candidates: "unsigned long", "unsigned int" or "uint32_t". I'm OK with any of those 3. Since Tcl only accepts maximum 28 bits for exponentation, any datatype which has at least 28 bits is suitable for me (strictly speaking, "int" could be 16 bits on some embedded platforms, but I don't think that Tcl still fits on such machines ....)

minad · 2019-06-13T14:41:10Z

@nijtmans Ok, thanks!

minad · 2019-07-03T11:20:52Z

@sjaeckel What about naming the functions mp_expt_u32 and mp_root_u32 here? ~~I also added the mp_get_mag_u* renamings.~~ (see #325)

sjaeckel · 2019-07-04T08:47:45Z

I just thought, what could happen, let's use the mp_expt() as is, but then you already added the prefix which is even better IMO ;-)

thanks!

minad · 2019-07-04T08:49:15Z

Hmm I still think without suffix is better. I just wanted to throw the option in for discussion.

minad · 2019-07-04T08:50:02Z

I don't care much about the name, but much more about the types and api stability. As far as I see it, the only downside of the _u32 suffix is that it suggests that more variants are available or are going to be added, which might not be the case.

sjaeckel · 2019-07-17T13:57:32Z

the only downside of the _u32 suffix is that it suggests that more variants are available or are going to be added, which might not be the case

IMO the clearer naming outweighs this downside, so I'll merge it

minad requested review from czurnieden and sjaeckel and removed request for czurnieden May 29, 2019 18:39

minad force-pushed the deprecate-expt-root branch from 4bb4d0d to 917fc2e Compare May 29, 2019 18:45

minad mentioned this pull request May 29, 2019

API discussions/overhaul #243

Closed

minad force-pushed the deprecate-expt-root branch 2 times, most recently from 7e93c41 to 837aa7e Compare May 29, 2019 19:17

czurnieden approved these changes May 29, 2019

View reviewed changes

minad mentioned this pull request May 31, 2019

First sketch for an easier configuration #301

Closed

sjaeckel reviewed May 31, 2019

View reviewed changes

minad force-pushed the deprecate-expt-root branch from 837aa7e to 455f30a Compare May 31, 2019 16:13

minad force-pushed the deprecate-expt-root branch 2 times, most recently from 2cc36bb to 1de2d6c Compare June 6, 2019 15:46

minad force-pushed the deprecate-expt-root branch from 1de2d6c to 7c84b08 Compare June 12, 2019 10:04

minad mentioned this pull request Jun 13, 2019

Provide explicit symbols for bn_xxx_l and bn_xxx_ll functions. #321

Merged

minad force-pushed the deprecate-expt-root branch from 7e1c0a2 to 5625f9f Compare July 3, 2019 11:00

minad force-pushed the deprecate-expt-root branch from 546e505 to 76cfa0e Compare July 3, 2019 12:58

sjaeckel force-pushed the deprecate-expt-root branch from 76cfa0e to cc7d7d9 Compare July 4, 2019 08:46

sjaeckel approved these changes Jul 4, 2019

View reviewed changes

minad added 2 commits July 17, 2019 15:45

deprecate mp_expt_d and mp_n_root in favor of mp_expt and mp_root

7169654

rename mp_root/mp_expt to mp_*_u32

ca89e9c

sjaeckel force-pushed the deprecate-expt-root branch from cc7d7d9 to ca89e9c Compare July 17, 2019 13:45

sjaeckel merged commit 584405f into develop Jul 17, 2019

sjaeckel deleted the deprecate-expt-root branch July 17, 2019 13:57

czurnieden mentioned this pull request Mar 7, 2023

Feature request: mp_expt #240

Closed

deprecate mp_expt_d and mp_n_root in favor of mp_expt and mp_root #304

deprecate mp_expt_d and mp_n_root in favor of mp_expt and mp_root #304

Uh oh!

Conversation

minad commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

czurnieden left a comment

Choose a reason for hiding this comment

Uh oh!

minad commented May 29, 2019

Uh oh!

sjaeckel May 31, 2019

Choose a reason for hiding this comment

Uh oh!

sjaeckel May 31, 2019

Choose a reason for hiding this comment

Uh oh!

minad May 31, 2019

Choose a reason for hiding this comment

Uh oh!

minad May 31, 2019

Choose a reason for hiding this comment

Uh oh!

sjaeckel May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minad May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minad May 31, 2019

Choose a reason for hiding this comment

Uh oh!

sjaeckel May 31, 2019

Choose a reason for hiding this comment

Uh oh!

minad May 31, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

minad commented Jun 4, 2019

Uh oh!

sjaeckel commented Jun 7, 2019

Uh oh!

minad commented Jun 7, 2019

Uh oh!

nijtmans commented Jun 13, 2019

Uh oh!

minad commented Jun 13, 2019

Uh oh!

minad commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjaeckel commented Jul 4, 2019

Uh oh!

minad commented Jul 4, 2019

Uh oh!

minad commented Jul 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjaeckel commented Jul 17, 2019

Uh oh!

Uh oh!

minad commented May 29, 2019 •

edited

Loading

sjaeckel May 31, 2019 •

edited

Loading

minad May 31, 2019 •

edited

Loading

minad commented Jul 3, 2019 •

edited

Loading

minad commented Jul 4, 2019 •

edited

Loading