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

[Philipp Stephani <p.stephani2@gmail.com>]

Am Mo., 18. Nov. 2019 um 22:21 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>
> 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.)

This sounds like a rather complex and probably premature optimization.
The current "unsigned long" type is identical to mpz_limb_t on most
platforms, and module authors can make use of that if they wish and
have profiled their code.

>
> 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?

With that argument you could argue that even fixed integer or float
conversions are unneeded because users could convert them to strings
as well. However, they are separate data types, so they deserve their
own 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?

Sure, but that's for a different commit.

>
> 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.

In practice the implementation can't change, since that would break
either source or binary compatibility.
I'm not against a typedef to provide clearer semantics, but then we'd
also have to define EMACS_LIMB_MAX etc., and I see relatively little
benefit in that (since we can't realistically change it anyway later).

>
> 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.

I'm not feeling strongly about the function in isolation, but the
current interface is modeled after the existing copy_string_contents
(which we can't change), and providing two different interfaces for
"copying an array" would be very confusing and unnecessarily
inconsistent.

>
>  > extracts the
>  > +integral value of @var{arg}.
>
> Need to make it clearer that ARG can be any integer; it need not be a
> bignum.

Ack, I'll make that change.

>
>  > 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.

I'm not sure we should document such facts. It's true that this allows
callers to use malloc without checking for overflow, and it seems this
already follows from the C standard (can an array with total size
larger than SIZE_MAX exist?), but then I'm not convinced that this is
reason enough to add more complicated guarantees here.

>
>  > +  bool success = env->extract_big_integer (env, value, &sign,
> &count, NULL);
>
> Shouldn't that 'value' be 'arg'?

Yeah, that follows from changing the naming conventions in the middle ;)

>
>  > +  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.