Re: [PATCH] Change module interface to no longer use GMP objects directly.

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 2642 bytes --]

Thanks. I like the idea of decoupling Emacs modules from GMP.

The proposed approach will typically require copying bignum data twice 
to do a conversion, even if modules use GMP themselves. Shouldn't there 
be a shortcut for modules using GMP, so that they can use 
mpz_limbs_write and mpz_limbs_finish and bypass one of the copies? We 
can do that even if the module interface itself doesn't include gmp.h or 
assume GMP. For example, the Emacs module interface could define a type 
emacs_limb_t that happens to be the same size and representation as 
mpz_limb_t when Emacs is implemented via GMP, and the example function 
could look like the attached handwavy file "bigt.c". (It might be 
possible to avoid even the remaining copy in some cases; I didn't 
investigate this.)

Without such a shortcut, then we are to some extent giving up on 
efficiency, and in that case shouldn't we just ask people to convert to 
text and back, thus simplifying the interface?

On 11/17/19 10:38 AM, Philipp Stephani wrote:

 > +@deftypefn Function bool extract_big_integer (emacs_env *@var{env}, 
emacs_value @var{arg}, int *@var{sign}, ptrdiff_t *@var{count}, unsigned 
long *@var{magnitude})
 > +This function, which is available since Emacs 27,

Should we be more systematic about saying which Emacs versions support 
which parts of the module API?

Uses of 'unsigned long' should be changed to a more-abstract integer 
type like 'emacs_limb_t', to give the implementation more leeway to 
change in future versions.

The extraction API looks a bit complicated. How about if we simplify it 
so that it (1) never signals an error and (2) it returns the limb count 
instead of always returning 'true' and storing the limb count via a 
pointer. Any too-small array can be reported merely by returning a limb 
count larger than the array size, without changing the array, as in the 
attached sample code.

 > extracts the
 > +integral value of @var{arg}.

Need to make it clearer that ARG can be any integer; it need not be a 
bignum.

 > it stores the required array
 > +size into @code{*count}

It should document that this size cannot exceed (min (PTRDIFF_MAX, 
SIZE_MAX) / sizeof (emacs_limb_t)). No matter what we do inside Emacs, 
it won't exceed that value for various reasons, and documenting this can 
simplify callers.

 > +  bool success = env->extract_big_integer (env, value, &sign, 
&count, NULL);

Shouldn't that 'value' be 'arg'?

 > +  mpz_import (value, count, order, size, endian, nails, magnitude);

Shouldn't that 'value' be 'result'?

At this point I stopped detailed review because I wanted your thoughts 
on the bigger questions.

[-- Attachment #2: bigt.c --]
[-- Type: text/x-csrc, Size: 880 bytes --]

  static bool
  extract_big_integer (emacs_env *env, emacs_value arg, mpz_t result)
  {
    int sign;
    unsigned long u;
    ptrdiff_t count = env->extract_big_integer (env, arg, &sign, 1, &u);
    if (count <= 1)
      mpz_set_ui (result, u);
    else
      {
	emacs_limb_t *magnitude
	  = _Generic (*mpz_limbs_write (result, count),
		      emacs_limb_t: mpz_limbs_write (result, count),
		      default: checked_malloc (count * sizeof *magnitude));
	env->extract_big_integer (env, arg, &sign, count, magnitude);
	if (_Generic (*mpz_limbs_write (result, count),
		      emacs_limb_t:
		        (mpz_limbs_finish (result, sign < 0 ? -count : count),
		         true),
		      default: false))
	  return true;
	mpz_import (result, count, -1, sizeof *magnitude, 0, 0, magnitude);
	free (magnitude);
      }

    if (sign < 0)
      mpz_neg (result, result);
    return true;
  }