Subject: bug#8525: closed (Re: bug#8546: fix for Emacs pseudovector incompatibility with GCC 4.6.0)

From: [email-masked] (GNU bug Tracking System)
Date: Tue, 26 Apr 2011 20:07:03 +0000

Your bug report

#8525: Lisp reader and string-to-number bugs and inconsistencies

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
8525: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8525
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#8546: fix for Emacs pseudovector incompatibility with GCC 4.6.0
From: Paul Eggert <[email-masked]>
To: Stefan Monnier <[email-masked]>
Cc: [email-masked], [email-masked]
Date: Tue, 26 Apr 2011 13:06:35 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

On 04/26/11 05:46, Stefan Monnier wrote:
 
> Ah, yes, that makes sense as well (and deserves another brief comment).

OK, thanks, I added comments for that, and for the other areas where
you requested comments, and merged it into the trunk.  I'll mark this
bug as done.  This merge also fixes bug 8525, which I'll also mark.

As mentioned in bug 8525, this fix assumes strtoumax and therefore
may require the Windows build to supply a 2-line inttypes.h if
Windows doesn't already have inttypes.h.  For details please see
<http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8525#14>.

Subject: Lisp reader and string-to-number bugs and inconsistencies
From: Paul Eggert <[email-masked]>
To: [email-masked]
Date: Wed, 20 Apr 2011 02:27:54 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

Emacs has several problems when converting strings to numbers:

1. On a typical 64-bit host, (string-to-number "2305843009213693951")
   returns 2305843009213693440, which is off by 511.  There are more
   subtle numeric errors due to double-rounding.

2. The Lisp reader sometimes reports integer overflow for large
   integers, and sometimes silently substitutes a float.  For example,
   on a typical 32-bit host, the Lisp reader reads 536870912 as if it
   were 536870912.0, but reports an overflow if it reads 2147483648.

3. The Lisp reader treats the tokens -. and +. as if they were 0, which
   is not documented and surely is not intended.

4. The Lisp reader parses NaNs and infinities, e.g., 0.0e+NaN is
   treated as a NaN; but (string-to-number "0.0e+NaN") returns zero.

I plan to install the following patch to fix these problems, after
some further testing and editing (right just now I noticed a stray
comment "Return the length of the floating-point number ...", which I
will remove).

To fix (2), it's plausible to change the code in one of two ways:
either silently treat large integers as floats, or signal an overflow.
I don't care that much one way or another, but Emacs should be
consistent.  I mildly prefer reporting the overflow, as that is a
better way to allow an upgrade path to arbitrary precision arithmetic,
so that's what the patch below does; but if the consensus is the other
way, I can easily change this.

# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: [email-masked]
# target_branch: bzr+ssh://[email-masked]/emacs/trunk
# testament_sha1: 4a86d5674868b293852101d1d3ad0a7bc157e65c
# timestamp: 2011-04-20 01:43:56 -0700
# base_revision_id: [email-masked]\
#   vk45j4qhfkv0xz4j
# 
# Begin patch
=== modified file 'src/ChangeLog'
--- src/ChangeLog	2011-04-19 10:48:30 +0000
+++ src/ChangeLog	2011-04-20 06:24:51 +0000
@@ -1,3 +1,28 @@
+2011-04-20  Paul Eggert  <[email-masked]>
+
+	Make the Lisp reader and string-to-float more consistent.
+	* data.c (atof): Remove decl; no longer used or needed.
+	(Fstring_to_number): Use new string_to_float function, to be
+	consistent with how the Lisp reader treats infinities and NaNs.
+	Do not assume that floating-point numbers represent EMACS_INT
+	without losing information; this is not true on most 64-bit hosts.
+	Avoid double-rounding errors, by insisting on integers when
+	parsing non-base-10 numbers, as the documentation specifies.
+	Report integer overflow instead of silently converting to
+	integers.
+	* lisp.h (string_to_float): New decl, replacing ...
+	(isfloat_string): Remove.
+	* lread.c (read1): Do not accept +. and -. as integers; this
+	appears to have been a coding error.  Similarly, do not accept
+	strings like +-1e0 as floating point numbers.  Do not report
+	overflow for some integer overflows and not others; instead,
+	report them all.  Break out the floating-point parsing into a new
+	function string_to_float, so that Fstring_to_number parses
+	floating point numbers consistently with the Lisp reader.
+	(string_to_float): New function, replacing isfloat_string.
+	This function checks for valid syntax and produces the resulting
+	Lisp float number too.
+
 2011-04-19  Eli Zaretskii  <[email-masked]>
 
 	* syntax.h (SETUP_SYNTAX_TABLE_FOR_OBJECT): Fix setting of

=== modified file 'src/data.c'
--- src/data.c	2011-04-16 21:48:36 +0000
+++ src/data.c	2011-04-20 06:24:51 +0000
@@ -48,10 +48,6 @@
 
 #include <math.h>
 
-#if !defined (atof)
-extern double atof (const char *);
-#endif /* !atof */
-
 Lisp_Object Qnil, Qt, Qquote, Qlambda, Qunbound;
 static Lisp_Object Qsubr;
 Lisp_Object Qerror_conditions, Qerror_message, Qtop_level;
@@ -2415,8 +2411,7 @@
 {
   register char *p;
   register int b;
-  int sign = 1;
-  Lisp_Object val;
+  EMACS_INT n;
 
   CHECK_STRING (string);
 
@@ -2430,38 +2425,23 @@
 	xsignal1 (Qargs_out_of_range, base);
     }
 
-  /* Skip any whitespace at the front of the number.  Some versions of
-     atoi do this anyway, so we might as well make Emacs lisp consistent.  */
+  /* Skip any whitespace at the front of the number.  Typically strtol does
+     this anyway, so we might as well be consistent.  */
   p = SSDATA (string);
   while (*p == ' ' || *p == '\t')
     p++;
 
-  if (*p == '-')
-    {
-      sign = -1;
-      p++;
-    }
-  else if (*p == '+')
-    p++;
-
-  if (isfloat_string (p, 1) && b == 10)
-    val = make_float (sign * atof (p));
-  else
-    {
-      double v = 0;
-
-      while (1)
-	{
-	  int digit = digit_to_number (*p++, b);
-	  if (digit < 0)
-	    break;
-	  v = v * b + digit;
-	}
-
-      val = make_fixnum_or_float (sign * v);
-    }
-
-  return val;
+  if (b == 10)
+    {
+      Lisp_Object val = string_to_float (p, 1);
+      if (FLOATP (val))
+	return val;
+    }
+
+  n = strtol (p, NULL, b);
+  if (FIXNUM_OVERFLOW_P (n))
+    xsignal (Qoverflow_error, list1 (string));
+  return make_number (n);
 }
 
 

=== modified file 'src/lisp.h'
--- src/lisp.h	2011-04-15 08:22:34 +0000
+++ src/lisp.h	2011-04-20 06:24:51 +0000
@@ -2782,7 +2782,7 @@
   } while (0)
 extern int openp (Lisp_Object, Lisp_Object, Lisp_Object,
                   Lisp_Object *, Lisp_Object);
-extern int isfloat_string (const char *, int);
+Lisp_Object string_to_float (char const *, int);
 extern void map_obarray (Lisp_Object, void (*) (Lisp_Object, Lisp_Object),
                          Lisp_Object);
 extern void dir_warning (const char *, Lisp_Object);

=== modified file 'src/lread.c'
--- src/lread.c	2011-04-14 05:04:02 +0000
+++ src/lread.c	2011-04-20 06:24:51 +0000
@@ -3006,85 +3006,32 @@
 	if (!quoted && !uninterned_symbol)
 	  {
 	    register char *p1;
+	    Lisp_Object result;
 	    p1 = read_buffer;
 	    if (*p1 == '+' || *p1 == '-') p1++;
 	    /* Is it an integer? */
-	    if (p1 != p)
+	    if ('0' <= *p1 && *p1 <= '9')
 	      {
-		while (p1 != p && (c = *p1) >= '0' && c <= '9') p1++;
+		do
+		  p1++;
+		while ('0' <= *p1 && *p1 <= '9');
+
 		/* Integers can have trailing decimal points.  */
-		if (p1 > read_buffer && p1 < p && *p1 == '.') p1++;
+		p1 += (*p1 == '.');
 		if (p1 == p)
-		  /* It is an integer. */
-		  {
-		    if (p1[-1] == '.')
-		      p1[-1] = '\0';
-		    {
-		      /* EMACS_INT n = atol (read_buffer); */
-		      char *endptr = NULL;
-		      EMACS_INT n = (errno = 0,
-				     strtol (read_buffer, &endptr, 10));
-		      if (errno == ERANGE && endptr)
-			{
-			  Lisp_Object args
-			    = Fcons (make_string (read_buffer,
-						  endptr - read_buffer),
-				     Qnil);
-			  xsignal (Qoverflow_error, args);
-			}
-		      return make_fixnum_or_float (n);
-		    }
-		  }
-	      }
-	    if (isfloat_string (read_buffer, 0))
-	      {
-		/* Compute NaN and infinities using 0.0 in a variable,
-		   to cope with compilers that think they are smarter
-		   than we are.  */
-		double zero = 0.0;
-
-		double value;
-
-		/* Negate the value ourselves.  This treats 0, NaNs,
-		   and infinity properly on IEEE floating point hosts,
-		   and works around a common bug where atof ("-0.0")
-		   drops the sign.  */
-		int negative = read_buffer[0] == '-';
-
-		/* The only way p[-1] can be 'F' or 'N', after isfloat_string
-		   returns 1, is if the input ends in e+INF or e+NaN.  */
-		switch (p[-1])
-		  {
-		  case 'F':
-		    value = 1.0 / zero;
-		    break;
-		  case 'N':
-		    value = zero / zero;
-
-		    /* If that made a "negative" NaN, negate it.  */
-
-		    {
-		      int i;
-		      union { double d; char c[sizeof (double)]; } u_data, u_minus_zero;
-
-		      u_data.d = value;
-		      u_minus_zero.d = - 0.0;
-		      for (i = 0; i < sizeof (double); i++)
-			if (u_data.c[i] & u_minus_zero.c[i])
-			  {
-			    value = - value;
-			    break;
-			  }
-		    }
-		    /* Now VALUE is a positive NaN.  */
-		    break;
-		  default:
-		    value = atof (read_buffer + negative);
-		    break;
-		  }
-
-		return make_float (negative ? - value : value);
-	      }
+		  {
+		    /* It is an integer. */
+		    EMACS_INT n = strtol (read_buffer, NULL, 10);
+		    if (FIXNUM_OVERFLOW_P (n))
+		      xsignal (Qoverflow_error,
+			       list1 (build_string (read_buffer)));
+		    return make_number (n);
+		  }
+	      }
+
+	    result = string_to_float (read_buffer, 0);
+	    if (FLOATP (result))
+	      return result;
 	  }
 	{
 	  Lisp_Object name, result;
@@ -3242,20 +3189,40 @@
 }
 
 
+/* Return the length of the floating-point number that is the prefix of CP, or
+   zero if there is none.  */
+
 #define LEAD_INT 1
 #define DOT_CHAR 2
 #define TRAIL_INT 4
 #define E_CHAR 8
 #define EXP_INT 16
 
-int
-isfloat_string (const char *cp, int ignore_trailing)
+
+/* Convert CP to a floating point number.  Return a non-float value if CP does
+   not have valid floating point syntax.  If IGNORE_TRAILING is nonzero,
+   consider just the longest prefix of CP that has valid floating point
+   syntax.  */
+
+Lisp_Object
+string_to_float (char const *cp, int ignore_trailing)
 {
   int state;
   const char *start = cp;
 
+  /* Compute NaN and infinities using a variable, to cope with compilers that
+     think they are smarter than we are.  */
+  double zero = 0;
+
+  /* Negate the value ourselves.  This treats 0, NaNs, and infinity properly on
+     IEEE floating point hosts, and works around a formerly-common bug where
+     atof ("-0.0") drops the sign.  */
+  int negative = *cp == '-';
+
+  double value = 0;
+
   state = 0;
-  if (*cp == '+' || *cp == '-')
+  if (negative || *cp == '+')
     cp++;
 
   if (*cp >= '0' && *cp <= '9')
@@ -3295,21 +3262,43 @@
     {
       state |= EXP_INT;
       cp += 3;
+      value = 1.0 / zero;
     }
   else if (cp[-1] == '+' && cp[0] == 'N' && cp[1] == 'a' && cp[2] == 'N')
     {
       state |= EXP_INT;
       cp += 3;
+      value = zero / zero;
+
+      /* If that made a "negative" NaN, negate it.  */
+      {
+	int i;
+	union { double d; char c[sizeof (double)]; } u_data, u_minus_zero;
+
+	u_data.d = value;
+	u_minus_zero.d = - 0.0;
+	for (i = 0; i < sizeof (double); i++)
+	  if (u_data.c[i] & u_minus_zero.c[i])
+	    {
+	      value = - value;
+	      break;
+	    }
+      }
+      /* Now VALUE is a positive NaN.  */
     }
 
-  return ((ignore_trailing
-	   || *cp == 0 || *cp == ' ' || *cp == '\t' || *cp == '\n'
-	   || *cp == '\r' || *cp == '\f')
-	  && (state == (LEAD_INT|DOT_CHAR|TRAIL_INT)
-	      || state == (DOT_CHAR|TRAIL_INT)
-	      || state == (LEAD_INT|E_CHAR|EXP_INT)
-	      || state == (LEAD_INT|DOT_CHAR|TRAIL_INT|E_CHAR|EXP_INT)
-	      || state == (DOT_CHAR|TRAIL_INT|E_CHAR|EXP_INT)));
+  if (! (state == (LEAD_INT|DOT_CHAR|TRAIL_INT)
+	 || state == (DOT_CHAR|TRAIL_INT)
+	 || state == (LEAD_INT|E_CHAR|EXP_INT)
+	 || state == (LEAD_INT|DOT_CHAR|TRAIL_INT|E_CHAR|EXP_INT)
+	 || state == (DOT_CHAR|TRAIL_INT|E_CHAR|EXP_INT)))
+    return make_number (0); /* Any non-float value will do.  */
+
+  if (! value)
+    value = atof (start + negative);
+  if (negative)
+    value = - value;
+  return make_float (value);
 }
 
 

# Begin bundle