min and max now return one of their arguments

* doc/lispref/numbers.texi (Comparison of Numbers):
* etc/NEWS: Document this.
* src/data.c (Amax, Amin): Remove constants.  All uses removed.
(minmax_driver): New function.
(Fmax, Fmin): Use it instead of arith_driver.
* test/src/data-tests.el (data-tests-max, data-tests-min): New tests.

doc/lispref/numbers.texi

@@ -427,8 +427,6 @@ the following argument.  It returns @code{t} if so, @code{nil} otherwise.
 
 @defun max number-or-marker &rest numbers-or-markers
 This function returns the largest of its arguments.
-If any of the arguments is floating point, the value is returned
-as floating point, even if it was given as an integer.
 
 @example
 (max 20)
@@ -436,14 +434,12 @@ as floating point, even if it was given as an integer.
 (max 1 2.5)
      @result{} 2.5
 (max 1 3 2.5)
-     @result{} 3.0
+     @result{} 3
 @end example
 @end defun
 
 @defun min number-or-marker &rest numbers-or-markers
 This function returns the smallest of its arguments.
-If any of the arguments is floating point, the value is returned
-as floating point, even if it was given as an integer.
 
 @example
 (min -4 1)

etc/NEWS

@@ -803,6 +803,13 @@ Unicode horizontal whitespace as defined in the Unicode Technical
 Standard #18.  If you only want to match space and tab, use [ \t]
 instead.
 
++++
+** 'min' and 'max' now always return one of their arguments.
+Formerly, they returned a floating-point value if any argument was
+floating-point, which was sometimes numerically incorrect.  For
+example, (min most-positive-fixnum (+ 1.0 most-positive-fixnum)) now
+always returns its first argument instead of its second.
+
 
 * Lisp Changes in Emacs 26.1
 

src/data.c

@@ -2719,9 +2719,7 @@ enum arithop
     Adiv,
     Alogand,
     Alogior,
-    Alogxor,
-    Amax,
-    Amin
+    Alogxor
   };
 
 static Lisp_Object float_arith_driver (double, ptrdiff_t, enum arithop,
@@ -2807,14 +2805,6 @@ arith_driver (enum arithop code, ptrdiff_t nargs, Lisp_Object *args)
 	case Alogxor:
 	  accum ^= next;
 	  break;
-	case Amax:
-	  if (!argnum || next > accum)
-	    accum = next;
-	  break;
-	case Amin:
-	  if (!argnum || next < accum)
-	    accum = next;
-	  break;
 	}
     }
 
@@ -2871,14 +2861,6 @@ float_arith_driver (double accum, ptrdiff_t argnum, enum arithop code,
 	case Alogior:
 	case Alogxor:
 	  wrong_type_argument (Qinteger_or_marker_p, val);
-	case Amax:
-	  if (!argnum || isnan (next) || next > accum)
-	    accum = next;
-	  break;
-	case Amin:
-	  if (!argnum || isnan (next) || next < accum)
-	    accum = next;
-	  break;
 	}
     }
 
@@ -2975,22 +2957,37 @@ Both X and Y must be numbers or markers.  */)
   return val;
 }
 
+static Lisp_Object
+minmax_driver (ptrdiff_t nargs, Lisp_Object *args,
+	       enum Arith_Comparison comparison)
+{
+  eassume (0 < nargs);
+  Lisp_Object accum;
+  for (ptrdiff_t argnum = 0; argnum < nargs; argnum++)
+    {
+      Lisp_Object val = args[argnum];
+      if (argnum == 0 || !NILP (arithcompare (val, accum, comparison)))
+	accum = val;
+      else if (FLOATP (accum) && isnan (XFLOAT_DATA (accum)))
+	break;
+    }
+  return accum;
+}
+
 DEFUN ("max", Fmax, Smax, 1, MANY, 0,
        doc: /* Return largest of all the arguments (which must be numbers or markers).
-The value is always a number; markers are converted to numbers.
 usage: (max NUMBER-OR-MARKER &rest NUMBERS-OR-MARKERS)  */)
   (ptrdiff_t nargs, Lisp_Object *args)
 {
-  return arith_driver (Amax, nargs, args);
+  return minmax_driver (nargs, args, ARITH_GRTR);
 }
 
 DEFUN ("min", Fmin, Smin, 1, MANY, 0,
        doc: /* Return smallest of all the arguments (which must be numbers or markers).
-The value is always a number; markers are converted to numbers.
 usage: (min NUMBER-OR-MARKER &rest NUMBERS-OR-MARKERS)  */)
   (ptrdiff_t nargs, Lisp_Object *args)
 {
-  return arith_driver (Amin, nargs, args);
+  return minmax_driver (nargs, args, ARITH_LESS);
 }
 
 DEFUN ("logand", Flogand, Slogand, 0, MANY, 0,

test/src/data-tests.el

@@ -80,6 +80,26 @@
   ;; Short circuits before getting to bad arg
   (should-not (>= 8 9 'foo)))
 
+(ert-deftest data-tests-max ()
+  (should-error (max))
+  (should (= 1 (max 1)))
+  (should (= 3 (max 3 2)))
+  (should (= 666 (max 666 1 0 0 -2 -3 -3 -3 -4 -8 -8 -9 -999)))
+  (should (= (1+ most-negative-fixnum)
+             (max (float most-negative-fixnum) (1+ most-negative-fixnum))))
+  (should (= 8 (apply #'max '(3 8 3))))
+  (should-error (max 9 8 'foo)))
+
+(ert-deftest data-tests-min ()
+  (should-error (min))
+  (should (= 1 (min 1)))
+  (should (= 2 (min 3 2)))
+  (should (= -999 (min 666 1 0 0 -2 -3 -3 -3 -4 -8 -8 -9 -999)))
+  (should (= most-positive-fixnum
+             (min (+ 1.0 most-positive-fixnum) most-positive-fixnum)))
+  (should (= 3 (apply #'min '(3 8 3))))
+  (should-error (min 9 8 'foo)))
+
 ;; Bool vector tests.  Compactly represent bool vectors as hex
 ;; strings.