bug#41347: 28.0.50; calculator.el: Cannot input negative exponents

bug#41347: 28.0.50; calculator.el: Cannot input negative exponents

[Chris Zheng]

Hello,

With current master, starting Emacs with `-Q`:

1. M-x calculator
2. Input 1e-3 RET

You will get 1 instead of 0.001. I believe this happens since Emacs 26.1. The root cause is the `calculator-string-to-number` function in lisp/calculator.el. Now the function gives

(calculator-string-to-number "1e-3") => 1.0
(calculator-string-to-number "1e3") => 1000.0
(calculator-string-to-number "1e") => 1.0

The funcional code is 

(replace-regexp-in-string
                  "[eE][+-]?\\([^0-9].*\\)?$" "e0\\1" str)

It changes `1e-3` to `1e0-3` that is recognized as 1. I have a possible fix attached below. Please see if it is correct. 

Thank you,

Chris Zheng



 From 7693d7072e4787c4b0663f490be5d83c1d9a6ee3 Mon Sep 17 00:00:00 2001
From: Chris Zheng <chriszheng99@gmail.com>
Date: Sun, 17 May 2020 13:43:35 +0800
Subject: [PATCH 1/1] Fix calculator exponent input

* lisp/calculator.el (calculator-string-to-number): Change the regexp
to correctly deal with negative exponents.
---
  lisp/calculator.el | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/calculator.el b/lisp/calculator.el
index 7e0b2fcc6a..fa3eb19099 100644
--- a/lisp/calculator.el
+++ b/lisp/calculator.el
@@ -863,7 +863,7 @@ calculator-string-to-number
      (let* ((str (replace-regexp-in-string
                   "\\.\\([^0-9].*\\)?$" ".0\\1" str))
             (str (replace-regexp-in-string
-                 "[eE][+-]?\\([^0-9].*\\)?$" "e0\\1" str)))
+                 "[eE]\\([+-]?\\)?$" "e\\10" str)))
        (float (string-to-number str)))))
  
  (defun calculator-push-curnum ()
-- 
2.16.1.windows.1

bug#41347: 28.0.50; calculator.el: Cannot input negative exponents

[Mattias Engdegård]

> @@ -863,7 +863,7 @@ calculator-string-to-number
>      (let* ((str (replace-regexp-in-string
>                   "\\.\\([^0-9].*\\)?$" ".0\\1" str))
>             (str (replace-regexp-in-string
> -                 "[eE][+-]?\\([^0-9].*\\)?$" "e0\\1" str)))
> +                 "[eE]\\([+-]?\\)?$" "e\\10" str)))
>        (float (string-to-number str)))))

Thanks for the report and the suggested patch! However, I'm not sure what either of these replace-regexp-in-string calls are good for. The first one possibly to accept 1.e23 instead of 1e23; the second one is less clear. Frankly, I think we can drop both.

Eli, do you remember?

bug#41347: 28.0.50; calculator.el: Cannot input negative exponents

[Andreas Schwab]

On Mai 17 2020, Mattias Engdegård wrote:

>> @@ -863,7 +863,7 @@ calculator-string-to-number
>>      (let* ((str (replace-regexp-in-string
>>                   "\\.\\([^0-9].*\\)?$" ".0\\1" str))
>>             (str (replace-regexp-in-string
>> -                 "[eE][+-]?\\([^0-9].*\\)?$" "e0\\1" str)))
>> +                 "[eE]\\([+-]?\\)?$" "e\\10" str)))
>>        (float (string-to-number str)))))
>
> Thanks for the report and the suggested patch! However, I'm not sure what either of these replace-regexp-in-string calls are good for. The first one possibly to accept 1.e23 instead of 1e23; the second one is less clear. Frankly, I think we can drop both.

In commit f248292ede, there was

-                ((string-match-p "[eE][+-]?$" str) (concat str "0"))

so the bug is that the part matching "[+-]?" is now dropped.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#41347: 28.0.50; calculator.el: Cannot input negative exponents

[Mattias Engdegård]

17 maj 2020 kl. 13.57 skrev Andreas Schwab <schwab@linux-m68k.org>:

> In commit f248292ede, there was
> 
> -                ((string-match-p "[eE][+-]?$" str) (concat str "0"))
> 
> so the bug is that the part matching "[+-]?" is now dropped.

In the commit you refer to, read-from-string was used; a partial input like "1e" had to be completed with an extra "0" to make the result a (floating-point) number. This no longer appears necessary since we use string-to-number and convert integers to float.

bug#41347: 28.0.50; calculator.el: Cannot input negative exponents

[Eli Barzilay]

On Sun, May 17, 2020 at 7:08 AM Mattias Engdegård <mattiase@acm.org> wrote:
>
> > @@ -863,7 +863,7 @@ calculator-string-to-number
> >      (let* ((str (replace-regexp-in-string
> >                   "\\.\\([^0-9].*\\)?$" ".0\\1" str))
> >             (str (replace-regexp-in-string
> > -                 "[eE][+-]?\\([^0-9].*\\)?$" "e0\\1" str)))
> > +                 "[eE]\\([+-]?\\)?$" "e\\10" str)))
> >        (float (string-to-number str)))))
>
> Thanks for the report and the suggested patch! However, I'm not sure
> what either of these replace-regexp-in-string calls are good for. The
> first one possibly to accept 1.e23 instead of 1e23; the second one is
> less clear. Frankly, I think we can drop both.
>
> Eli, do you remember?

Sidenote: there's not much point in the double "?" in "\\([+-]?\\)?".

But more to the point, I don't remember why I switched to the regexp
mess in the first place.  The original code:

    (car (read-from-string
          (cond ((equal "." str) "0.0")
                ((string-match "[eE][+-]?$" str) (concat str "0"))
                ((string-match "\\.[0-9]\\|[eE]" str) str)
                ((string-match "\\." str)
                 ;; do this because Emacs reads "23." as an integer
                 (concat str "0"))
                ((stringp str) (concat str ".0"))
                (t "0.0"))))

makes the intention clear -- the idea is to mimic common calculators
where you can type "3." or "3e" and get 3.  (Re the fix from a short
while ago, the comment also shows that the original intention was to
always get a float.)

It looks like going back to a simplified (due to the float) version of
this would be better.  Also testing that the thing I mentioned in the
log still works ("e+" using "+" as an operator).

-- 
                   ((x=>x(x))(x=>x(x)))                  Eli Barzilay:
                   http://barzilay.org/                  Maze is Life!

bug#41347: 28.0.50; calculator.el: Cannot input negative exponents

[Mattias Engdegård]

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

17 maj 2020 kl. 22.26 skrev Eli Barzilay <eli@barzilay.org>:

> the idea is to mimic common calculators
> where you can type "3." or "3e" and get 3.

Thank you Eli! I can confirm that after removing all the string transformation prior to the call to string-to-number, everything works as expected except "1.e3" (dot before E). A single transformation taking care of that case was added for the sake of completeness.

The attached patch has now been pushed to master.


[-- Attachment #2: 0001-Fix-calculator-entry-of-numbers-with-negative-expone.patch --]
[-- Type: application/octet-stream, Size: 1574 bytes --]

From 482baa9856e4269bdf8f253621013592bc8de5b2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sun, 17 May 2020 18:11:27 +0200
Subject: [PATCH] Fix calculator entry of numbers with negative exponents
 (bug#41347)

* lisp/calculator.el (calculator-string-to-number):
Remove obsolete string transformations preventing entry of 1e-3 etc.
Keep one transformation to allow entry of "1.e3".
Reported by Chris Zheng.
---
 lisp/calculator.el | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/lisp/calculator.el b/lisp/calculator.el
index 7e0b2fcc6a..cd92f99268 100644
--- a/lisp/calculator.el
+++ b/lisp/calculator.el
@@ -858,12 +858,10 @@ calculator-string-to-number
   "Convert the given STR to a number, according to the value of
 `calculator-input-radix'."
   (if calculator-input-radix
-    (string-to-number str (cadr (assq calculator-input-radix
-                                      '((bin 2) (oct 8) (hex 16)))))
-    (let* ((str (replace-regexp-in-string
-                 "\\.\\([^0-9].*\\)?$" ".0\\1" str))
-           (str (replace-regexp-in-string
-                 "[eE][+-]?\\([^0-9].*\\)?$" "e0\\1" str)))
+      (string-to-number str (cadr (assq calculator-input-radix
+                                        '((bin 2) (oct 8) (hex 16)))))
+    ;; Allow entry of "1.e3".
+    (let ((str (replace-regexp-in-string (rx "." (any "eE")) "e" str)))
       (float (string-to-number str)))))
 
 (defun calculator-push-curnum ()
-- 
2.21.1 (Apple Git-122.3)

bug#41347: 28.0.50; calculator.el: Cannot input negative exponents

[Chris Zheng]

On Mon, May 18, 2020 at 11:28:21AM +0200, Mattias Engdegård wrote:
>17 maj 2020 kl. 22.26 skrev Eli Barzilay <eli@barzilay.org>:
>
>> the idea is to mimic common calculators
>> where you can type "3." or "3e" and get 3.
>
>Thank you Eli! I can confirm that after removing all the string transformation prior to the call to string-to-number, everything works as expected except "1.e3" (dot before E). A single transformation taking care of that case was added for the sake of completeness.
>
>The attached patch has now been pushed to master.

Hello Mattias,

Thank you for fixing it. Still I think it is uncommon to use `rx` here...

Best regards,

Chris

bug#41347: 28.0.50; calculator.el: Cannot input negative exponents

[Mattias Engdegård]

18 maj 2020 kl. 17.01 skrev Chris Zheng <chriszheng99@gmail.com>:

> Thank you for fixing it. Still I think it is uncommon to use `rx` here...

Yes, in this case rx doesn't improve the readability much. It doesn't hurt, though, does it?
It's just become a habit of mine to use rx everywhere. Try it you too!

bug#41347: 28.0.50; calculator.el: Cannot input negative exponents

[Eli Barzilay]

Thanks!  -- I'll try to get to testing it soon-ish...

On Mon, May 18, 2020 at 5:28 AM Mattias Engdegård <mattiase@acm.org> wrote:
>
> 17 maj 2020 kl. 22.26 skrev Eli Barzilay <eli@barzilay.org>:
>
> > the idea is to mimic common calculators
> > where you can type "3." or "3e" and get 3.
>
> Thank you Eli! I can confirm that after removing all the string transformation prior to the call to string-to-number, everything works as expected except "1.e3" (dot before E). A single transformation taking care of that case was added for the sake of completeness.
>
> The attached patch has now been pushed to master.
>


-- 
                   ((x=>x(x))(x=>x(x)))                  Eli Barzilay:
                   http://barzilay.org/                  Maze is Life!