Re: Bignum speedup patch causes crash at startup

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

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

Eli Zaretskii wrote:
> I don't understand why it prefers GMP calls to a simple double
> division, when both arguments are fixnums.  What am I missing?

First, converting a fixnum to a double can lose info. Second, the double 
division could round, which would mean the result would be double-rounded. We're 
already suffering from double-rounding when either argument is floating-point, 
and we don't want that bug to also infect integer division. So even the Emacs 26 
code (which doesn't have bignums) must use integer division here, not double 
division.

This is worth commenting so I installed the attached. While writing the comment 
I noticed there's a bug when one argument is a bignum and the other is a float, 
so I fixed that in the attached too.

[-- Attachment #2: 0001-Fix-round-FLOAT-BIGNUM-bug.patch --]
[-- Type: text/x-patch, Size: 2091 bytes --]

From 21637d5e5b29d5ec8fb966c0ddfbfba3eb33da38 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 4 Sep 2018 11:49:41 -0700
Subject: [PATCH] Fix (round FLOAT BIGNUM) bug

* src/floatfns.c (rounding_driver): Fix bug when one
argument is a float and the other is a bignum.
* test/src/floatfns-tests.el (bignum-round): Test for the bug.
---
 src/floatfns.c             | 7 +++++--
 test/src/floatfns-tests.el | 5 +++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/floatfns.c b/src/floatfns.c
index 2f33b8652b..13ab7b0359 100644
--- a/src/floatfns.c
+++ b/src/floatfns.c
@@ -355,6 +355,8 @@ rounding_driver (Lisp_Object arg, Lisp_Object divisor,
       CHECK_NUMBER (divisor);
       if (!FLOATP (arg) && !FLOATP (divisor))
 	{
+	  /* Divide as integers.  Converting to double might lose
+	     info, even for fixnums; also see the FIXME below.  */
 	  if (EQ (divisor, make_fixnum (0)))
 	    xsignal0 (Qarith_error);
 	  int_divide (mpz[0],
@@ -363,10 +365,11 @@ rounding_driver (Lisp_Object arg, Lisp_Object divisor,
 	  return make_integer_mpz ();
 	}
 
-      double f1 = FLOATP (arg) ? XFLOAT_DATA (arg) : XFIXNUM (arg);
-      double f2 = FLOATP (divisor) ? XFLOAT_DATA (divisor) : XFIXNUM (divisor);
+      double f1 = XFLOATINT (arg);
+      double f2 = XFLOATINT (divisor);
       if (! IEEE_FLOATING_POINT && f2 == 0)
 	xsignal0 (Qarith_error);
+      /* FIXME: This division rounds, so the result is double-rounded.  */
       d = f1 / f2;
     }
 
diff --git a/test/src/floatfns-tests.el b/test/src/floatfns-tests.el
index d41b08f796..9a382058b4 100644
--- a/test/src/floatfns-tests.el
+++ b/test/src/floatfns-tests.el
@@ -70,6 +70,11 @@
       (should (= n (floor n)))
       (should (= n (round n)))
       (should (= n (truncate n)))
+      (let ((-n (- n))
+	    (f (float n))
+	    (-f (- (float n))))
+	(should (= 1 (round n f) (round -n -f) (round f n) (round -f -n)))
+	(should (= -1 (round -n f) (round n -f) (round f -n) (round -f n))))
       (dolist (d ns)
         (let ((q (/ n d))
               (r (% n d))
-- 
2.17.1