unofficial mirror of bug-guile@gnu.org 
 help / color / mirror / Atom feed
From: Timothy Sample <samplet@ngyro.com>
To: 51276@debbugs.gnu.org
Subject: bug#51276: Problems with format and scaling floats
Date: Mon, 18 Oct 2021 17:22:49 -0400	[thread overview]
Message-ID: <87tuhejady.fsf@ngyro.com> (raw)

[-- 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


             reply	other threads:[~2021-10-18 21:22 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-18 21:22 Timothy Sample [this message]
2021-10-19  1:02 ` bug#51276: Problems with format and scaling floats lloda
2021-10-19  8:42 ` Bengt Richter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/guile/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87tuhejady.fsf@ngyro.com \
    --to=samplet@ngyro.com \
    --cc=51276@debbugs.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).