Removing some workarounds for big integers

Removing some workarounds for big integers

[Philipp]

Hi,

now that Emacs has big integers, would it make sense to remove some of
the workarounds for lack of big integer support that are still present?
Specifically, the ‘make_fixnum_or_float’ and ‘INTEGER_TO_CONS’ macros.
A replacement that would generate a fixnum or bignum depending on the
input value seems strictly superior.  (It would require a bit more
memory, but I don’t think memory pressure is an issue here.)

Philipp

Re: Removing some workarounds for big integers

[John Wiegley]

>>>>> "P" == Philipp  <p.stephani2@gmail.com> writes:

P> now that Emacs has big integers, would it make sense to remove some of the
P> workarounds for lack of big integer support that are still present?
P> Specifically, the ‘make_fixnum_or_float’ and ‘INTEGER_TO_CONS’ macros. A
P> replacement that would generate a fixnum or bignum depending on the input
P> value seems strictly superior. (It would require a bit more memory, but I
P> don’t think memory pressure is an issue here.)

Sounds reasonable to me.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Removing some workarounds for big integers

[Andy Moreton]

On Mon 20 Aug 2018, Philipp wrote:

> Hi,
>
> now that Emacs has big integers, would it make sense to remove some of
> the workarounds for lack of big integer support that are still present?
> Specifically, the ‘make_fixnum_or_float’ and ‘INTEGER_TO_CONS’ macros.
> A replacement that would generate a fixnum or bignum depending on the
> input value seems strictly superior.  (It would require a bit more
> memory, but I don’t think memory pressure is an issue here.)

Yes, but that requires identifying all of the callers, and converting
them to support bignums. All fixable, but work to be done.

    AndyM

Re: Removing some workarounds for big integers

[Philipp Stephani]

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

Andy Moreton <andrewjmoreton@gmail.com> schrieb am Di., 21. Aug. 2018 um
00:59 Uhr:

> On Mon 20 Aug 2018, Philipp wrote:
>
> > Hi,
> >
> > now that Emacs has big integers, would it make sense to remove some of
> > the workarounds for lack of big integer support that are still present?
> > Specifically, the ‘make_fixnum_or_float’ and ‘INTEGER_TO_CONS’ macros.
> > A replacement that would generate a fixnum or bignum depending on the
> > input value seems strictly superior.  (It would require a bit more
> > memory, but I don’t think memory pressure is an issue here.)
>
> Yes, but that requires identifying all of the callers, and converting
> them to support bignums. All fixable, but work to be done.
>

It looks like Paul has already done most of this work in commit
d77d01d22902acdc45c2c7059de4f1b158ab5806.

[-- Attachment #2: Type: text/html, Size: 1181 bytes --]

Re: Removing some workarounds for big integers

[Paul Eggert]

Philipp Stephani wrote:

> It looks like Paul has already done most of this work in commit
> d77d01d22902acdc45c2c7059de4f1b158ab5806.

Yes, I've already replaced all uses of make_fixnum_or_float and INTEGER_TO_CONS 
with INT_TO_INTEGER.

There are still quite a few places that need looking at, though. Stefan 
mentioned timestamps; I'll try to bump the priority of that.

Re: Removing some workarounds for big integers

[Philipp Stephani]

Am Sa., 22. Sept. 2018 um 01:12 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>
> Philipp Stephani wrote:
>
> > It looks like Paul has already done most of this work in commit
> > d77d01d22902acdc45c2c7059de4f1b158ab5806.
>
> Yes, I've already replaced all uses of make_fixnum_or_float and INTEGER_TO_CONS
> with INT_TO_INTEGER.
>
> There are still quite a few places that need looking at, though. Stefan
> mentioned timestamps; I'll try to bump the priority of that.

I've checked some of the remaining uses of CONS_TO_INTEGER. It seems
some of them we can replace right away. For some others the
documentation requires a number; we can detect integral floats for
them, but should deprecate the float usage.

Re: Removing some workarounds for big integers

[Andy Moreton]

On Mon 22 Apr 2019, Philipp Stephani wrote:

> Am Sa., 22. Sept. 2018 um 01:12 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>>
>> Philipp Stephani wrote:
>>
>> > It looks like Paul has already done most of this work in commit
>> > d77d01d22902acdc45c2c7059de4f1b158ab5806.
>>
>> Yes, I've already replaced all uses of make_fixnum_or_float and INTEGER_TO_CONS
>> with INT_TO_INTEGER.
>>
>> There are still quite a few places that need looking at, though. Stefan
>> mentioned timestamps; I'll try to bump the priority of that.
>
> I've checked some of the remaining uses of CONS_TO_INTEGER. It seems
> some of them we can replace right away. For some others the
> documentation requires a number; we can detect integral floats for
> them, but should deprecate the float usage.

In commit 4e2ea400 ("Introduce a helper macro to convert a Lisp integer
to a C integer."):

+/* Return the integral value of NUM.  If NUM is too big for TYPE,
+   signal an error.  */
+#define INTEGER_TO_INT(num, type)                                              \
+  (TYPE_SIGNED (type)                                                          \
+     ? ranged_integer_to_int ((num), TYPE_MINIMUM (type), TYPE_MAXIMUM (type)) \
+     : ranged_integer_to_uint ((num), TYPE_MINIMUM (type)))
                                       ^^^^^^^^^^^^
This should be TYPE_MAXIMUM.

    AndyM

Re: Removing some workarounds for big integers

[Philipp Stephani]

Am Mo., 22. Apr. 2019 um 18:56 Uhr schrieb Andy Moreton
<andrewjmoreton@gmail.com>:
>
> On Mon 22 Apr 2019, Philipp Stephani wrote:
>
> > Am Sa., 22. Sept. 2018 um 01:12 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
> >>
> >> Philipp Stephani wrote:
> >>
> >> > It looks like Paul has already done most of this work in commit
> >> > d77d01d22902acdc45c2c7059de4f1b158ab5806.
> >>
> >> Yes, I've already replaced all uses of make_fixnum_or_float and INTEGER_TO_CONS
> >> with INT_TO_INTEGER.
> >>
> >> There are still quite a few places that need looking at, though. Stefan
> >> mentioned timestamps; I'll try to bump the priority of that.
> >
> > I've checked some of the remaining uses of CONS_TO_INTEGER. It seems
> > some of them we can replace right away. For some others the
> > documentation requires a number; we can detect integral floats for
> > them, but should deprecate the float usage.
>
> In commit 4e2ea400 ("Introduce a helper macro to convert a Lisp integer
> to a C integer."):
>
> +/* Return the integral value of NUM.  If NUM is too big for TYPE,
> +   signal an error.  */
> +#define INTEGER_TO_INT(num, type)                                              \
> +  (TYPE_SIGNED (type)                                                          \
> +     ? ranged_integer_to_int ((num), TYPE_MINIMUM (type), TYPE_MAXIMUM (type)) \
> +     : ranged_integer_to_uint ((num), TYPE_MINIMUM (type)))
>                                        ^^^^^^^^^^^^
> This should be TYPE_MAXIMUM.


Thanks, fixed

Re: Removing some workarounds for big integers

[Paul Eggert]

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

On 4/22/19 9:59 AM, Philipp Stephani wrote:
>
>> +#define INTEGER_TO_INT(num, type)                                              \
>> +  (TYPE_SIGNED (type)                                                          \
>> +     ? ranged_integer_to_int ((num), TYPE_MINIMUM (type), TYPE_MAXIMUM (type)) \
>> +     : ranged_integer_to_uint ((num), TYPE_MINIMUM (type)))
>>                                        ^^^^^^^^^^^^
>> This should be TYPE_MAXIMUM.
>
> Thanks, fixed
>
More important, INTEGER_TO_INT's type conversion messes up and can cause
a signal on picky platforms. I installed the attached patch to undo that
change. Although lisp.lh really does need a better API for integer
conversion (and I'll volunteer to review/help if someone wants to work
on that!), INTEGER_TO_INT is not a step in the right direction; we need
something more general/accurate/whatever.


[-- Attachment #2: 0001-Go-back-to-old-way-of-checking-json-int-range.patch --]
[-- Type: text/x-patch, Size: 3075 bytes --]

From c2dcf5d671142b2fc9ddb1142aa498a2d5aa46ec Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 22 Apr 2019 11:40:13 -0700
Subject: [PATCH] Go back to old way of checking json int range

Although the lisp.h macros really need improvement,
INTEGER_TO_INT is not the right way to go about it, as it
causes conversion from intmax_t to uintmax_t and back again,
which can cause a signal if the value is negative.
* src/lisp.h (INTEGER_TO_INT, ranged_integer_to_int)
(ranged_integer_to_uint): Remove, reverting recent changes to
this file.
* src/json.c (lisp_to_json): Revert to previous code,
as the change messes up with uintmax_t<->intmax_t conversion.
---
 src/json.c |  9 ++++++++-
 src/lisp.h | 27 ---------------------------
 2 files changed, 8 insertions(+), 28 deletions(-)

diff --git a/src/json.c b/src/json.c
index 16500bce72..256d485eea 100644
--- a/src/json.c
+++ b/src/json.c
@@ -495,7 +495,14 @@ lisp_to_json (Lisp_Object lisp, struct json_configuration *conf)
   else if (EQ (lisp, Qt))
     return json_check (json_true ());
   else if (INTEGERP (lisp))
-    return json_check (json_integer (INTEGER_TO_INT (lisp, json_int_t)));
+    {
+      intmax_t low = TYPE_MINIMUM (json_int_t);
+      intmax_t high = TYPE_MAXIMUM (json_int_t);
+      intmax_t value;
+      if (! (integer_to_intmax (lisp, &value) && low <= value && value <= high))
+        args_out_of_range_3 (lisp, make_int (low), make_int (high));
+      return json_check (json_integer (value));
+    }
   else if (FLOATP (lisp))
     return json_check (json_real (XFLOAT_DATA (lisp)));
   else if (STRINGP (lisp))
diff --git a/src/lisp.h b/src/lisp.h
index ee5a8481ae..d803f16000 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -2640,13 +2640,6 @@ make_uint (uintmax_t n)
 #define INT_TO_INTEGER(expr) \
   (EXPR_SIGNED (expr) ? make_int (expr) : make_uint (expr))
 
-/* Return the integral value of NUM.  If NUM is too big for TYPE,
-   signal an error.  */
-#define INTEGER_TO_INT(num, type)                                              \
-  (TYPE_SIGNED (type)                                                          \
-     ? ranged_integer_to_int ((num), TYPE_MINIMUM (type), TYPE_MAXIMUM (type)) \
-     : ranged_integer_to_uint ((num), TYPE_MAXIMUM (type)))
-
 \f
 /* Forwarding pointer to an int variable.
    This is allowed only in the value cell of a symbol,
@@ -5023,26 +5016,6 @@ maybe_gc (void)
     garbage_collect ();
 }
 
-INLINE intmax_t
-ranged_integer_to_int (Lisp_Object num, intmax_t min, intmax_t max)
-{
-  CHECK_INTEGER (num);
-  intmax_t result;
-  if (!(integer_to_intmax (num, &result) && min <= result && result <= max))
-    args_out_of_range_3 (num, make_int (min), make_int (max));
-  return result;
-}
-
-INLINE uintmax_t
-ranged_integer_to_uint (Lisp_Object num, uintmax_t max)
-{
-  CHECK_INTEGER (num);
-  uintmax_t result;
-  if (!(integer_to_uintmax (num, &result) && result <= max))
-    args_out_of_range_3 (num, make_fixed_natnum (0), make_uint (max));
-  return result;
-}
-
 INLINE_HEADER_END
 
 #endif /* EMACS_LISP_H */
-- 
2.20.1

Re: Removing some workarounds for big integers

[Paul Eggert]

On 4/22/19 8:40 AM, Philipp Stephani wrote:
> I've checked some of the remaining uses of CONS_TO_INTEGER. It seems
> some of them we can replace right away. For some others the
> documentation requires a number; we can detect integral floats for
> them, but should deprecate the float usage.

It sounds like a good idea to move in that direction. However, wouldn't
it be simpler to modify cons_to_unsigned and cons_to_signed to issue
warnings now, for every argument that is not an integer? I don't offhand
see why some uses of these functions can be replaced right away while
others require gentler handling. Won't it be easier on users (simpler
documentation, etc.) if we treat all these functions alike during the
conversion?

Re: Removing some workarounds for big integers

[Philipp Stephani]

Am Mo., 22. Apr. 2019 um 20:45 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>
> On 4/22/19 9:59 AM, Philipp Stephani wrote:
> >
> >> +#define INTEGER_TO_INT(num, type)                                              \
> >> +  (TYPE_SIGNED (type)                                                          \
> >> +     ? ranged_integer_to_int ((num), TYPE_MINIMUM (type), TYPE_MAXIMUM (type)) \
> >> +     : ranged_integer_to_uint ((num), TYPE_MINIMUM (type)))
> >>                                        ^^^^^^^^^^^^
> >> This should be TYPE_MAXIMUM.
> >
> > Thanks, fixed
> >
> More important, INTEGER_TO_INT's type conversion messes up and can cause
> a signal on picky platforms.

How so?


> Although lisp.lh really does need a better API for integer
> conversion (and I'll volunteer to review/help if someone wants to work
> on that!), INTEGER_TO_INT is not a step in the right direction; we need
> something more general/accurate/whatever.

I don't really understand. What could be more general than converting
to an arbitrary integer type? Also, how could the conversion be more
accurate than representing each representable value and signaling an
error otherwise?

Re: Removing some workarounds for big integers

[Philipp Stephani]

Am Mo., 22. Apr. 2019 um 20:57 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>
> On 4/22/19 8:40 AM, Philipp Stephani wrote:
> > I've checked some of the remaining uses of CONS_TO_INTEGER. It seems
> > some of them we can replace right away. For some others the
> > documentation requires a number; we can detect integral floats for
> > them, but should deprecate the float usage.
>
> It sounds like a good idea to move in that direction. However, wouldn't
> it be simpler to modify cons_to_unsigned and cons_to_signed to issue
> warnings now, for every argument that is not an integer?

I don't think such warnings would work that easily. They would only be
shown to the end user, not the developer making the deprecated call.
Most of the time the conses come from some other function and the
developer just passes them on.

Re: Removing some workarounds for big integers

[Paul Eggert]

On 8/1/20 1:09 PM, Philipp Stephani wrote:
> Am Mo., 22. Apr. 2019 um 20:45 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>>
>> On 4/22/19 9:59 AM, Philipp Stephani wrote:
>>>
>>>> +#define INTEGER_TO_INT(num, type)                                              \
>>>> +  (TYPE_SIGNED (type)                                                          \
>>>> +     ? ranged_integer_to_int ((num), TYPE_MINIMUM (type), TYPE_MAXIMUM (type)) \
>>>> +     : ranged_integer_to_uint ((num), TYPE_MINIMUM (type)))
>>>>                                         ^^^^^^^^^^^^
>>>> This should be TYPE_MAXIMUM.
>>>
>>> Thanks, fixed
>>>
>> More important, INTEGER_TO_INT's type conversion messes up and can cause
>> a signal on picky platforms.
> 
> How so?

The type conversion is messed up because on conventional platforms 
INTEGER_TO_INT returns a value of uintmax_t, which means that an expression like 
'INTEGER_TO_INT (n, t) < 0' will always be false, even if N is negative and T is 
a signed type.

The "picky platform" is one where conversion from unsigned to signed signals 
when the value is out of range for the signed type; this behavior is allowed by 
POSIX and the C standard and I imagine some debugging implementations might 
check for it. On these implementations,

To work around this problem, the macro could have another argument, being the 
lvalue destination; that would avoid these problems. However, it'd be more 
awkward to use. At some point it's easier to avoid the macro and use the 
underlying functions.

Re: Removing some workarounds for big integers

[Philipp Stephani]

Am Mi., 5. Aug. 2020 um 08:11 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>
> On 8/1/20 1:09 PM, Philipp Stephani wrote:
> > Am Mo., 22. Apr. 2019 um 20:45 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
> >>
> >> On 4/22/19 9:59 AM, Philipp Stephani wrote:
> >>>
> >>>> +#define INTEGER_TO_INT(num, type)                                              \
> >>>> +  (TYPE_SIGNED (type)                                                          \
> >>>> +     ? ranged_integer_to_int ((num), TYPE_MINIMUM (type), TYPE_MAXIMUM (type)) \
> >>>> +     : ranged_integer_to_uint ((num), TYPE_MINIMUM (type)))
> >>>>                                         ^^^^^^^^^^^^
> >>>> This should be TYPE_MAXIMUM.
> >>>
> >>> Thanks, fixed
> >>>
> >> More important, INTEGER_TO_INT's type conversion messes up and can cause
> >> a signal on picky platforms.
> >
> > How so?
>
> The type conversion is messed up because on conventional platforms
> INTEGER_TO_INT returns a value of uintmax_t, which means that an expression like
> 'INTEGER_TO_INT (n, t) < 0' will always be false, even if N is negative and T is
> a signed type.
>
> The "picky platform" is one where conversion from unsigned to signed signals
> when the value is out of range for the signed type; this behavior is allowed by
> POSIX and the C standard and I imagine some debugging implementations might
> check for it. On these implementations,

Yeah, good point, I forgot about the annoying behavior of the ternary operator.

>
> To work around this problem, the macro could have another argument, being the
> lvalue destination; that would avoid these problems. However, it'd be more
> awkward to use. At some point it's easier to avoid the macro and use the
> underlying functions.

I wouldn't agree to that. The large advantage of packaging the
conversion up into a single macro is that the range check is always
correct and only needs to be coded once. Repeating the range check for
each caller is rather error-prone. So I think a void-returning macro
makes sense. It wouldn't even need another argument, as the type
follows from the destination. (At least if __typeof__ is available.)

Re: Removing some workarounds for big integers

[Philipp Stephani]

Am So., 9. Aug. 2020 um 17:39 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>

> >
> > To work around this problem, the macro could have another argument, being the
> > lvalue destination; that would avoid these problems. However, it'd be more
> > awkward to use. At some point it's easier to avoid the macro and use the
> > underlying functions.
>
> I wouldn't agree to that. The large advantage of packaging the
> conversion up into a single macro is that the range check is always
> correct and only needs to be coded once. Repeating the range check for
> each caller is rather error-prone. So I think a void-returning macro
> makes sense. It wouldn't even need another argument, as the type
> follows from the destination. (At least if __typeof__ is available.)

Actually it might be easier to enumerate all integer types using
_Generic. Since we now require C11, that should always be available.

Re: Removing some workarounds for big integers

[Stefan Monnier]

> Since we now require C11, that should always be available.

We do?


        Stefan

Re: Removing some workarounds for big integers

[Paul Eggert]

On 8/9/20 10:04 AM, Stefan Monnier wrote:
>> Since we now require C11, that should always be available.
> We do?

No, we require many (but not all) features of C99. We can use some C11 features 
if available, but we don't require any of them.

Re: Removing some workarounds for big integers

[Philipp Stephani]

Am So., 9. Aug. 2020 um 19:26 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>
> On 8/9/20 10:04 AM, Stefan Monnier wrote:
> >> Since we now require C11, that should always be available.
> > We do?
>
> No, we require many (but not all) features of C99. We can use some C11 features
> if available, but we don't require any of them.

Fair enough. The current approach with explicit type specifications is
then probably the best we can do for now.