minux <minux...@gmail.com> writes:
> Because we don't have separate docs for mini-gmp, and mini-gmp does
> provide mp_set_memory_functions, I think it's best if it can match
> gmp's behavior here to avoid confusions.
The differences are documented in mini-gmp/README (and your patch fixes
all but one of the documented differences).
> OK, patch updated. Now mini-gmp keeps track of old_size correctly and
> implements the behavior exactly as documented. And I have modified
> tests to ensure old_size is correct on realloc and free.
> I hope the added complexity is not too much.
Changes to mini-gmp.c look reasonable to me. Some detailed comments
below.
> In fact, I think it makes the code of mpq_get_str slightly easier to
> read.
I'm not that familiar with the mpq functions. I hope Marco can comment.
> diff -r bcca14c8a090 mini-gmp/mini-gmp.c
> --- a/mini-gmp/mini-gmp.c Thu Mar 05 00:43:26 2020 +0100
> +++ b/mini-gmp/mini-gmp.c Sat Mar 07 13:04:26 2020 -0500
> @@ -352,7 +352,8 @@
> }
>
> #define gmp_xalloc(size) ((*gmp_allocate_func)((size)))
> -#define gmp_free(p) ((*gmp_free_func) ((p), 0))
> +#define gmp_xfree(p, size) ((*gmp_free_func) ((p), (size)))
> +#define gmp_xrealloc(ptr, old_size, size) ((*gmp_reallocate_func)(ptr,
> old_size, size))
>
> static mp_ptr
> gmp_xalloc_limbs (mp_size_t size)
> @@ -361,10 +362,17 @@
> }
>
> static mp_ptr
> -gmp_xrealloc_limbs (mp_ptr old, mp_size_t size)
> +gmp_xrealloc_limbs (mp_ptr old, mp_size_t old_size, mp_size_t size)
> {
> + const size_t sl = sizeof (mp_limb_t);
> assert (size > 0);
> - return (mp_ptr) (*gmp_reallocate_func) (old, 0, size * sizeof (mp_limb_t));
> + return (mp_ptr) gmp_xrealloc (old, old_size * sl, size * sl);
> +}
I'd prefer repeating sizeof (mp_limb_t), instead of the local sl
constant.
> @@ -956,11 +964,12 @@
> mp_limb_t d, di;
> mp_limb_t r;
> mp_ptr tp = NULL;
> + mp_size_t tps = 0;
>
> if (inv->shift > 0)
> {
> /* Shift, reusing qp area if possible. In-place shift if qp == np. */
> - tp = qp ? qp : gmp_xalloc_limbs (nn);
> + tp = qp ? qp : gmp_xalloc_limbs (tps = nn);
Please write the assignment tps = nn as a separate statement. Also, I
think name "tn" instead of "tps" would be more consistent with
surrounding code.
> r = mpn_lshift (tp, np, nn, inv->shift);
> np = tp;
> }
> @@ -978,7 +987,7 @@
> qp[nn] = q;
> }
> if ((inv->shift > 0) && (tp != qp))
> - gmp_free (tp);
> + gmp_xfree_limbs (tp, tps);
Can the condition be simplified to just
if (tps > 0)
gmp_xfree_limbs (tp, tps);
?
> @@ -4143,7 +4152,7 @@
> size_t
> mpz_sizeinbase (const mpz_t u, int base)
> {
> - mp_size_t un;
> + mp_size_t un, oun;
> mp_srcptr up;
> mp_ptr tp;
> mp_bitcnt_t bits;
> @@ -4176,7 +4185,7 @@
> 10. */
> }
>
> - tp = gmp_xalloc_limbs (un);
> + tp = gmp_xalloc_limbs (oun = un);
I'd prefer
tn = un;
as a separate statement. And maybe clearer to leave un unchanged below, and
use tn as the loop variable?
> mpn_copyi (tp, up, un);
> mpn_div_qr_1_invert (&bi, base);
>
> @@ -4189,7 +4198,7 @@
> }
> while (un > 0);
>
> - gmp_free (tp);
> + gmp_xfree_limbs (tp, oun);
> return ndigits;
> }
>
> @@ -4199,7 +4208,7 @@
> unsigned bits;
> const char *digits;
> mp_size_t un;
> - size_t i, sn;
> + size_t i, sn, osn = 0;
>
> digits = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
> if (base > 1)
> @@ -4220,15 +4229,15 @@
>
> sn = 1 + mpz_sizeinbase (u, base);
> if (!sp)
> - sp = (char *) gmp_xalloc (1 + sn);
> + sp = (char *) gmp_xalloc (osn = 1 + sn);
I'd prefer
if (!sp)
{
osn = 1 + sn;
sp = (char *) gmp_xalloc (osn);
}
else
osn = 0;
(and drop the zero initialization at the declaration above).
> +ret:
> sp[sn] = '\0';
> + if (osn) sp = gmp_xrealloc(sp, osn, sn + 1);
> return sp;
> }
Would it make sense to also check if realloc is needed?
if (osn && osn != sn + 1)
gmp_xrealloc(...);
> @@ -4305,7 +4316,7 @@
> r->_mp_size = 0;
> return -1;
> }
> - dp = (unsigned char *) gmp_xalloc (strlen (sp));
> + dp = (unsigned char *) gmp_xalloc (dpn = strlen (sp));
dpn = strlen (sp);
as a separate statement. And I'm not that fond of the name dpn. Maybe
sn = strlen (sp);
and reorganize the loop as
for (i = 0; i < sn; i++)
{
...
dp[i] = digit;
}
> @@ -4382,7 +4393,7 @@
> str = mpz_get_str (NULL, base, x);
> len = strlen (str);
> len = fwrite (str, 1, len, stream);
> - gmp_free (str);
> + gmp_xfree (str, len + 1);
> return len;
> }
This seems wrong in the case of i/o errors, where fwrite may return a
shorter item count.
> --- a/mini-gmp/tests/testutils.c Thu Mar 05 00:43:26 2020 +0100
> +++ b/mini-gmp/tests/testutils.c Sat Mar 07 13:04:26 2020 -0500
> @@ -84,31 +84,51 @@
> static void *
> tu_realloc (void *p, size_t old_size, size_t new_size)
> {
> - size_t *block = block_check (p);
> - block = (size_t *) realloc (block, sizeof(size_t) + new_size +
> sizeof(block_end));
> + size_t *old_block = block_check (p), *block;
> + void *new;
> + if (old_block[0] != old_size)
> + {
> + fprintf (stderr, "%s:%d: bad old_size: want %ld, got %ld.\n",
> __FILE__, __LINE__,
> + (long)old_block[0], (long)old_size);
> + abort ();
> + }
> +
> + /* We do not use realloc here to test custom allocators where realloc
> + requires old_size to be set to the length of data that needs to be
> + copied from old to the newly allocated space. */
I think the above check of old_size should be enough for testing
mini-gmp. One can still use realloc for the actual allocation, no manual
memcpy.
Regards,
/Niels
--
Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677.
Internet email is subject to wholesale government surveillance.
_______________________________________________
gmp-devel mailing list
gmp-devel@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-devel
