bug#51276: Problems with format and scaling floats

bug#51276: Problems with format and scaling floats

[Timothy Sample]

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

Hi Guilers,

It turns out there’s a little blunder in ‘format’ (from ‘ice-9’).  Look
at what happens when using the SCALE argument to format a fixed-point
float (this is Guile from the Git repo at the time of writing):

    GNU Guile 3.0.7.6-22120
    Copyright (C) 1995-2021 Free Software Foundation, Inc.

    Guile comes with ABSOLUTELY NO WARRANTY; for details type `,show w'.
    This program is free software, and you are welcome to redistribute it
    under certain conditions; type `,show c' for details.

    Enter `,help' for help.
    scheme@(guile-user)> (format #t "~,,3f~%" 0.00123)
    0.23
    $3 = #t
    scheme@(guile-user)> (format #t "~,,1f~%" 0.00123)
    ice-9/boot-9.scm:1685:16: In procedure raise-exception:
    Value out of range 0 to 400: -1

    Entering a new prompt.  Type `,bt' for a backtrace or `,q' to continue.

The first example gives the wrong result.  Scaling 0.00123 by 3 should
yield 1.23, not 0.23.  For the second example, instead of 0.0123, we get
an error!  What’s going on here?

Well, our ‘format’ code comes from SLIB and was written in 1998, so it’s
not easy to explain.  There’s so much mutation even a C programmer would
blush!  ;)  The issue happens in the ‘format:parse-float’ procedure
(which is defined inside of ‘format’).  It normalizes the string
representation of a number, and applies the scale argument when needed.
It does this by keeping a string of digits and the location of the
decimal point.  Another thing it keeps track of the leading zeros in a
variable called ‘left-zeros’.  Here’s the code that does the final
shifting and places the decimal point:

    (if (> left-zeros 0)
        (if (<= left-zeros shift) ; shift always > 0 here
            (format:fn-shiftleft shift) ; shift out 0s
            (begin
              (format:fn-shiftleft left-zeros)
              (set! format:fn-dot (- shift left-zeros))))
        (set! format:fn-dot (+ format:fn-dot shift)))

The issue is that the cases in the inner ‘if’ form are reversed.  That
is, if there are MORE leading zeros than we need to shift, we can just
shift.  Otherwise (if there are FEWER leading zeros), we need to shift
out the zeros and then move the decimal point (‘format:fn-dot’).

AFAICS, this bug was in the original SLIB implementation (1998) and has
not been fixed since then.  It’s been in Guile since 1999.

Anyway, that’s more than anyone cares to know....  Here’s a patch with
tests!  :)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ice-9-format-Fix-scaling-floats-with-leading-zeros.patch --]
[-- Type: text/x-patch, Size: 2337 bytes --]

From c31d1f5d44343da1201ea1be86bc6b2ac8af6c8d Mon Sep 17 00:00:00 2001
From: Timothy Sample <samplet@ngyro.com>
Date: Mon, 18 Oct 2021 17:07:41 -0400
Subject: [PATCH] ice-9 format: Fix scaling floats with leading zeros

---
 module/ice-9/format.scm      |  4 ++--
 test-suite/tests/format.test | 10 ++++++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/module/ice-9/format.scm b/module/ice-9/format.scm
index 48d9c0c84..ee7cba910 100644
--- a/module/ice-9/format.scm
+++ b/module/ice-9/format.scm
@@ -1359,10 +1359,10 @@
                       (else
                        (if (> left-zeros 0)
                            (if (<= left-zeros shift) ; shift always > 0 here
-                               (format:fn-shiftleft shift) ; shift out 0s
                                (begin
                                  (format:fn-shiftleft left-zeros)
-                                 (set! format:fn-dot (- shift left-zeros))))
+                                 (set! format:fn-dot (- shift left-zeros)))
+                               (format:fn-shiftleft shift)) ; shift out 0s
                            (set! format:fn-dot (+ format:fn-dot shift))))))))
 
                (let ((negexp          ; expon format m.nnnEee
diff --git a/test-suite/tests/format.test b/test-suite/tests/format.test
index b9aa7a854..d5111f1c6 100644
--- a/test-suite/tests/format.test
+++ b/test-suite/tests/format.test
@@ -2,7 +2,7 @@
 ;;;; Matthias Koeppe <mkoeppe@mail.math.uni-magdeburg.de> --- June 2001
 ;;;;
 ;;;; Copyright (C) 2001, 2003, 2004, 2006, 2010, 2011, 2012,
-;;;;   2014, 2017 Free Software Foundation, Inc.
+;;;;   2014, 2017, 2021 Free Software Foundation, Inc.
 ;;;;
 ;;;; This library is free software; you can redistribute it and/or
 ;;;; modify it under the terms of the GNU Lesser General Public
@@ -121,7 +121,13 @@
   ;; in guile prior to 1.6.9 and 1.8.1, leading zeros were incorrectly
   ;; stripped, moving the decimal point and giving "25.0" here
   (pass-if "string 02.5"
-    (string=? "2.5" (format #f "~f" "02.5"))))
+    (string=? "2.5" (format #f "~f" "02.5")))
+
+  (pass-if "scale with few leading zeros"
+    (string=? "1.23" (format #f "~,,3f" "0.00123")))
+
+  (pass-if "scale with many leading zeros"
+    (string=? "0.0123" (format #f "~,,1f" "0.00123"))))
 
 ;;;
 ;;; ~h
-- 
2.33.0

bug#51276: Problems with format and scaling floats

[lloda]

Thank you very much, patch applied!

Now that you've dived into (ice-9 format), what about this one:

(format #t "~,,2f~%" 0.01)
=> 1.0
(format #t "~,,3f~%" 0.01) ; !!
=> 010.0
(format #t "~,,3f~%" 0.001)
=> 1.0
(format #t "~,,4f~%" 0.001) ; !!
=> 0010.0
(format #t "~,,4f~%" 0.0001)
=> 1.0
(format #t "~,,5f~%" 0.0001) ; ok somehow...
=> 10.0

regards


> On 18 Oct 2021, at 23:22, Timothy Sample <samplet@ngyro.com> wrote:
> 
> Hi Guilers,
> 
> It turns out there’s a little blunder in ‘format’ (from ‘ice-9’).  Look
> at what happens when using the SCALE argument to format a fixed-point
> float (this is Guile from the Git repo at the time of writing):
> 
>    GNU Guile 3.0.7.6-22120
>    Copyright (C) 1995-2021 Free Software Foundation, Inc.
> 
>    Guile comes with ABSOLUTELY NO WARRANTY; for details type `,show w'.
>    This program is free software, and you are welcome to redistribute it
>    under certain conditions; type `,show c' for details.
> 
>    Enter `,help' for help.
>    scheme@(guile-user)> (format #t "~,,3f~%" 0.00123)
>    0.23
>    $3 = #t
>    scheme@(guile-user)> (format #t "~,,1f~%" 0.00123)
>    ice-9/boot-9.scm:1685:16: In procedure raise-exception:
>    Value out of range 0 to 400: -1
> 
>    Entering a new prompt.  Type `,bt' for a backtrace or `,q' to continue.
> 
> The first example gives the wrong result.  Scaling 0.00123 by 3 should
> yield 1.23, not 0.23.  For the second example, instead of 0.0123, we get
> an error!  What’s going on here?
> 
> Well, our ‘format’ code comes from SLIB and was written in 1998, so it’s
> not easy to explain.  There’s so much mutation even a C programmer would
> blush!  ;)  The issue happens in the ‘format:parse-float’ procedure
> (which is defined inside of ‘format’).  It normalizes the string
> representation of a number, and applies the scale argument when needed.
> It does this by keeping a string of digits and the location of the
> decimal point.  Another thing it keeps track of the leading zeros in a
> variable called ‘left-zeros’.  Here’s the code that does the final
> shifting and places the decimal point:
> 
>    (if (> left-zeros 0)
>        (if (<= left-zeros shift) ; shift always > 0 here
>            (format:fn-shiftleft shift) ; shift out 0s
>            (begin
>              (format:fn-shiftleft left-zeros)
>              (set! format:fn-dot (- shift left-zeros))))
>        (set! format:fn-dot (+ format:fn-dot shift)))
> 
> The issue is that the cases in the inner ‘if’ form are reversed.  That
> is, if there are MORE leading zeros than we need to shift, we can just
> shift.  Otherwise (if there are FEWER leading zeros), we need to shift
> out the zeros and then move the decimal point (‘format:fn-dot’).
> 
> AFAICS, this bug was in the original SLIB implementation (1998) and has
> not been fixed since then.  It’s been in Guile since 1999.
> 
> Anyway, that’s more than anyone cares to know....  Here’s a patch with
> tests!  :)
> 
> From c31d1f5d44343da1201ea1be86bc6b2ac8af6c8d Mon Sep 17 00:00:00 2001
> From: Timothy Sample <samplet@ngyro.com>
> Date: Mon, 18 Oct 2021 17:07:41 -0400
> Subject: [PATCH] ice-9 format: Fix scaling floats with leading zeros
> 
> ---
> module/ice-9/format.scm      |  4 ++--
> test-suite/tests/format.test | 10 ++++++++--
> 2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/module/ice-9/format.scm b/module/ice-9/format.scm
> index 48d9c0c84..ee7cba910 100644
> --- a/module/ice-9/format.scm
> +++ b/module/ice-9/format.scm
> @@ -1359,10 +1359,10 @@
>                       (else
>                        (if (> left-zeros 0)
>                            (if (<= left-zeros shift) ; shift always > 0 here
> -                               (format:fn-shiftleft shift) ; shift out 0s
>                                (begin
>                                  (format:fn-shiftleft left-zeros)
> -                                 (set! format:fn-dot (- shift left-zeros))))
> +                                 (set! format:fn-dot (- shift left-zeros)))
> +                               (format:fn-shiftleft shift)) ; shift out 0s
>                            (set! format:fn-dot (+ format:fn-dot shift))))))))
> 
>                (let ((negexp          ; expon format m.nnnEee
> diff --git a/test-suite/tests/format.test b/test-suite/tests/format.test
> index b9aa7a854..d5111f1c6 100644
> --- a/test-suite/tests/format.test
> +++ b/test-suite/tests/format.test
> @@ -2,7 +2,7 @@
> ;;;; Matthias Koeppe <mkoeppe@mail.math.uni-magdeburg.de> --- June 2001
> ;;;;
> ;;;; Copyright (C) 2001, 2003, 2004, 2006, 2010, 2011, 2012,
> -;;;;   2014, 2017 Free Software Foundation, Inc.
> +;;;;   2014, 2017, 2021 Free Software Foundation, Inc.
> ;;;;
> ;;;; This library is free software; you can redistribute it and/or
> ;;;; modify it under the terms of the GNU Lesser General Public
> @@ -121,7 +121,13 @@
>   ;; in guile prior to 1.6.9 and 1.8.1, leading zeros were incorrectly
>   ;; stripped, moving the decimal point and giving "25.0" here
>   (pass-if "string 02.5"
> -    (string=? "2.5" (format #f "~f" "02.5"))))
> +    (string=? "2.5" (format #f "~f" "02.5")))
> +
> +  (pass-if "scale with few leading zeros"
> +    (string=? "1.23" (format #f "~,,3f" "0.00123")))
> +
> +  (pass-if "scale with many leading zeros"
> +    (string=? "0.0123" (format #f "~,,1f" "0.00123"))))
> 
> ;;;
> ;;; ~h
> -- 
> 2.33.0
> 

bug#51276: Problems with format and scaling floats

[Bengt Richter]

Nice catch :)

On +2021-10-18 17:22:49 -0400, Timothy Sample wrote:
> Hi Guilers,
> 
> It turns out there’s a little blunder in ‘format’ (from ‘ice-9’).  Look
> at what happens when using the SCALE argument to format a fixed-point
> float (this is Guile from the Git repo at the time of writing):
> 
>     GNU Guile 3.0.7.6-22120
>     Copyright (C) 1995-2021 Free Software Foundation, Inc.
> 
>     Guile comes with ABSOLUTELY NO WARRANTY; for details type `,show w'.
>     This program is free software, and you are welcome to redistribute it
>     under certain conditions; type `,show c' for details.
> 
>     Enter `,help' for help.
>     scheme@(guile-user)> (format #t "~,,3f~%" 0.00123)
>     0.23
>     $3 = #t
>     scheme@(guile-user)> (format #t "~,,1f~%" 0.00123)
>     ice-9/boot-9.scm:1685:16: In procedure raise-exception:
>     Value out of range 0 to 400: -1
> 
>     Entering a new prompt.  Type `,bt' for a backtrace or `,q' to continue.
> 
> The first example gives the wrong result.  Scaling 0.00123 by 3 should
> yield 1.23, not 0.23.  For the second example, instead of 0.0123, we get
> an error!  What’s going on here?
> 
> Well, our ‘format’ code comes from SLIB and was written in 1998, so it’s
> not easy to explain.  There’s so much mutation even a C programmer would
> blush!  ;)  The issue happens in the ‘format:parse-float’ procedure
> (which is defined inside of ‘format’).  It normalizes the string
> representation of a number, and applies the scale argument when needed.
> It does this by keeping a string of digits and the location of the
> decimal point.  Another thing it keeps track of the leading zeros in a
> variable called ‘left-zeros’.  Here’s the code that does the final
> shifting and places the decimal point:
> 
>     (if (> left-zeros 0)
>         (if (<= left-zeros shift) ; shift always > 0 here
>             (format:fn-shiftleft shift) ; shift out 0s
>             (begin
>               (format:fn-shiftleft left-zeros)
>               (set! format:fn-dot (- shift left-zeros))))
>         (set! format:fn-dot (+ format:fn-dot shift)))
> 
> The issue is that the cases in the inner ‘if’ form are reversed.  That
> is, if there are MORE leading zeros than we need to shift, we can just
> shift.  Otherwise (if there are FEWER leading zeros), we need to shift
> out the zeros and then move the decimal point (‘format:fn-dot’).
> 
> AFAICS, this bug was in the original SLIB implementation (1998) and has
> not been fixed since then.  It’s been in Guile since 1999.
> 
> Anyway, that’s more than anyone cares to know....  Here’s a patch with
> tests!  :)
> 

1999 until now (2021), /and/ reliably reproduced all that time! :)

-- 
Regards,
Bengt Richter