added integer log with positive integer base #174

czurnieden · 2019-03-09T15:07:06Z

Example for a simple design to add a group of opt-in (independent) functions.

minad · 2019-03-16T08:49:02Z

What is the reasoning for the extra function macro?

czurnieden · 2019-03-16T12:04:15Z

What is the reasoning for the extra function macro?

Leftovers. Residual echoes.
Because it was all Macro in the beginning.
No, really, I have made the whole thing with preprocessor directives a couple of years ago! ;-)
(Not in the segmented version, a "standard" sieve only)

And I found another one: #define LTM_SIEVE_SIZE(bst) ((bst)->size) is useless, the struct entry is already named "size", no need for a "better name" macro.

Will tackle it after lunch.

czurnieden · 2019-03-28T00:22:15Z

Oh, I broke the couch? I know I need to loose some weight, Travis, no need to put a finger on it!

sjaeckel

first part of the review, more to come later or so

sjaeckel · 2019-03-29T08:03:34Z

bn_mp_factors_zero.c

+
+   if (f->factors != NULL) {
+      for (i = 0; i < f->length; i++) {
+         mp_zero(&(f->factors[i]));


that's not required, mp_clear() also zeroes the mp before clearing

first part of the review, more to come later or so

Uh oh! ;-)

that's not required, mp_clear() also zeroes the mp before clearing

Yepp, residue from trying out if mp_zero would suffice here (playing with the heap is always costly) but that results in quite a mess which I found out quickly.

sjaeckel · 2019-03-29T08:06:26Z

bn_mp_factor.c

+         }
+
+         if (all == MP_NO) {
+#if 0


can this code go or is there a reason this should stay?

The first one is useful for some numbers but will hold up everything for others and I'm benchmarking the other one if it is useful at that place. I'm thinking about adding a flag for the first one to let the caller decide. The latter is needed for the quadratic sieve, but that will come later, much later.

So: should it stay or should it go? Good question. (Why am I hearing The Clash in my head now?)

Oh, nearly forgot: both, mp_isperfpower and mp_ispower need a mp_n_root_ex with a normal runtime to begin with. It does not need the optimized version I have in a different branch (nthroot I think?) but at least the one I put into this PR. It implements just the standard seed (a bit above the actual root) and a slightly more paranoid check at the end. The usability of the original version is so small, I would consider the one in this PR nothing more than a bug-fix.

sjaeckel · 2019-03-29T08:06:41Z

bn_mp_factor.c

+   }
+
+   /* Switched off temporarily */
+#if 0


sjaeckel · 2019-03-29T08:07:41Z

bn_mp_decr.c

+ */
+
+/* Decrement "a" by one like "a--". Changes input! */
+#ifdef LTM_USE_EXTRA_FUNCTIONS


IMO this macro can be removed

It changes the input, not a very common thing in LibTomMath, so I was a bit cautious here.

But if that (remove the macro for this function and this function only) is not what you meant drop me a note please.

sjaeckel · 2019-03-29T08:16:58Z

bn_mp_factors_init.c

+#ifdef LTM_USE_EXTRA_FUNCTIONS
+int mp_factors_init(mp_factors *f)
+{
+   f->factors = OPT_CAST(mp_int) XMALLOC(sizeof(mp_int) * (size_t)(LTM_TRIAL_GROWTH));


this should be cleared, shouldn't it? so it could be a XCALLOC()!?

which brings me directly into... why aren't mp_init() and mp_init_size() using XCALLOC()?

this should be cleared, shouldn't it?

It is more or less just the backbone and gets overwritten. On the other side: better save than sorry and it doesn't cost much, so I'll clear it.

which brings me directly into... why aren't mp_init() and mp_init_size() using XCALLOC()?

It might have had a reason once but now? Yes. would support a change to XCALLOC here.

Lunchbreak wasn't shortened, so I was able to rebase before I really need to run and that reminded me that XCALLOC isn't in LTM anymore. Do you want to put it back in?

sjaeckel · 2019-03-29T08:26:32Z

bn_mp_factors_sort.c

+   for (i = 1 ; i < f->length; i++) {
+      idx = i;
+      while ((idx > 0) && (mp_cmp(&(f->factors[idx-1]),&(f->factors[idx])) == MP_GT)) {
+         if ((e = mp_copy(&(f->factors[idx]), &tmp)) != MP_OKAY) {


we're deep inside the library, why should we do a deep copy of all mp_int elements where it would be sufficient to exchange the mp_int?

Another leftover. I always start with mp_copy and check later if it can be replaced with mp_exch.

Thanks for taking the time to go through the whole mess!
(Lunchtime is a bit short today, the whole thing needs to wait till tonight.)

…dded tests for mp_n_root and mp_expt(_d)

czurnieden · 2019-03-30T00:33:24Z

Docs still need updates, sorry.
@sjaeckel I used calloc directly for now, didn't want to reimplement the 'XCALLOC' macro (yet).

minad · 2019-04-02T11:19:14Z

This one is also huge. Maybe splitting it would help? @sjaeckel What do you think?

czurnieden · 2019-04-02T18:34:20Z

@minad Not all functions are independent from each other in this PR. Do you want one PR per function? Has it pros and cons but the work for me is roughly the same, no matter what, so it's up to you to decide.

What I can split off without any problems are
bn_mp_decr.c, bn_mp_incr.c, bn_mp_expt.c, bn_mp_ilogb.c, and bn_mp_pollard_rho.c

Small packets:

bn_mp_miller_bach.c and bn_mp_prime_is_prime_deterministic.c (could be merged into one function or at least one file)
(bn_mp_n_root_ex.c, bn_mp_ispower.c, bn_mp_isperfpower.c Note: the changes for nth-root in this PR are the same as in changed seed to make nth-root usable #189 and are needed to make the is*power functions work.

The rest are more or less dependent of each other to various degrees, see callgraph.txt for all the gory details.

minad · 2019-04-02T18:43:36Z

Yes I understand that there are dependencies. Maybe multiple PRs in reverse topological order. No single functions but maybe logical units. This PR adds nearly 20000 lines, that's all I saw when I looked at it first ;)

sjaeckel · 2019-04-02T21:41:55Z

First I'd like to see this LTM_USE_EXTRA_FUNCTIONS macro gone

there are no "extra functions" it's in the library or it isn't :-)

and AFAICT for now it looks like this doesn't break backwards compati

Maybe multiple PRs in reverse topological order. No single functions but maybe logical units

👍

I'd start with the simple implementations and keep the complicated stuff for the last PR's

~~the change to mp_n_root_ex is independent to all this other stuff, right? so this could be a first quick PR~~ I've seen changed seed to make nth-root usable #189 now
mp_decr + mp_expt + mp_incr
mp_ilogb
mp_sieve_clear + mp_sieve_init + (why is there no mp_sieve in bn_mp_sieve.c but a function called mp_is_small_prime? :-D )
mp_next_small_prime + mp_prec_small_prime
mp_ispower + mp_miller_bach

the remaining stuff is ... well interdependent... probably it can at least commit-wise be split up into the factors-related functions and mp_primorial + mp_small_prime_array + mp_trial ... or something like that...

yes, that's a long way, but it's also 20k LOC

czurnieden · 2019-04-02T22:48:56Z

there are no "extra functions" it's in the library or it isn't :-)

Now, that's something I can work with! ;-)

and AFAICT for now it looks like this doesn't break backwards compati

That was intentional. Anything not backwards compatible in my code shall be considered a bug and it would be nice if I get dropped a note.
Breaking things is reserved for Version 2.0 ;-)

(why is there no mp_sieve in bn_mp_sieve.c but a function called mp_is_small_prime? :-D )

Because programmers are really bad at naming things! ;-)
It was originally all in that one file and mp_is_small_prime is the only function left after the Big De-cluttering.
But you are right, of course. The files in LTM are named after the (exported) function therein, so it should be renamed. Thankfully, renaming a file in LTM is really easy

yes, that's a long way,

I will have a free weekend this weekend (he says boldly), should suffice for a large part of that work.

but it's also 20k LOC

That much? Really? Oh.

minad · 2019-05-29T19:27:59Z

@czurnieden Can we close this one? ilogb, incr and decr are already merged. You are proposing more things but I lost sight somehow. I guess you want to reopen smaller separate PRs for those parts.

czurnieden · 2019-05-29T21:13:36Z

Can we close this one?

Yes. (thought it was already?)

You are proposing more things but I lost sight somehow

Yeah, it's a bit of a mess, admitted.
I cleaned my PRs as far as possible up but that's already a couple of weeks ago, they will will get another scrubbing these days.

czurnieden force-pushed the extrafunctions branch from 3a5db59 to 0814610 Compare March 27, 2019 20:49

sjaeckel reviewed Mar 29, 2019

View reviewed changes

czurnieden added 16 commits March 29, 2019 15:06

added integer log with positive integer base

670636f

additional corrections for MP_8BIT

ecd6a02

added prime-sieve (segmented) and documentation

4d2312f

replaced function macros with actual functions

9207a58

addition of mp_expt; fixed mp_n_root_ex in general and for MP_8BIT; a…

da2c192

…dded tests for mp_n_root and mp_expt(_d)

fixed memory leak

c6cfcae

put tests back in order

d078d7c

added factoring

7ef98d3

added testing for the extra functions to Travis

57d20b8

added deterministic primality test

2aa2cf6

reinstalled all tests

ff88ce9

additional castings to test.c

f4df64e

resolved memory problem

5b82a22

removed extraneous delimiter

64f3528

bugfixes and cleanups

1020f43

bugfixes and cleanup (API change in the sieve-functions!)

da630d8

czurnieden force-pushed the extrafunctions branch from d5bc46a to da630d8 Compare March 29, 2019 23:06

czurnieden added 2 commits March 30, 2019 00:19

caught a rogue #endif and put it back in its place

1758eaf

added necessary macro

39f8693

documentation update

f3a464d

minad added the work in progress label May 21, 2019

minad closed this May 29, 2019

added integer log with positive integer base #174

added integer log with positive integer base #174

Uh oh!

Conversation

czurnieden commented Mar 9, 2019

Uh oh!

minad commented Mar 16, 2019

Uh oh!

czurnieden commented Mar 16, 2019

Uh oh!

czurnieden commented Mar 28, 2019

Uh oh!

sjaeckel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

czurnieden commented Mar 30, 2019

Uh oh!

minad commented Apr 2, 2019

Uh oh!

czurnieden commented Apr 2, 2019

Uh oh!

minad commented Apr 2, 2019

Uh oh!

sjaeckel commented Apr 2, 2019

Uh oh!

czurnieden commented Apr 2, 2019

Uh oh!

minad commented May 29, 2019

Uh oh!

czurnieden commented May 29, 2019

Uh oh!

Uh oh!