Bug in org-table--set-calc-mode?

Bug in org-table--set-calc-mode?

[Daniele Nicolodi]

Hello,

I am hacking org-table-eval-formula (see thread about monetary values in
org-tables) which uses this inline function:

(defsubst org-table--set-calc-mode (var &optional value)
  (if (stringp var)
      (setq var (assoc var '(("D" calc-angle-mode deg)
			     ("R" calc-angle-mode rad)
			     ("F" calc-prefer-frac t)
			     ("S" calc-symbolic-mode t)))
	    value (nth 2 var) var (nth 1 var)))
  (if (memq var org-tbl-calc-modes)
      (setcar (cdr (memq var org-tbl-calc-modes)) value)
    (cons var (cons value org-tbl-calc-modes)))
  org-tbl-calc-modes)

which I am not able to understand or which is not correct.

The first (if ...) does some value substitutions, however, IIUC the
second (if ...) sets a new value for an entry in the org-tbl-calc-modes
plist if the entry is already present and builds a new plist with the
entry prepended if the entry is not there. However, the original plist
is returned and not the one with the new entry prepended.

It does not seem to be the intended behavior.

Shouldn't this be simply:

(defsubst org-table--set-calc-mode (var &optional value)
  (if (stringp var)
      (setq var (assoc var '(("D" calc-angle-mode deg)
			     ("R" calc-angle-mode rad)
			     ("F" calc-prefer-frac t)
			     ("S" calc-symbolic-mode t)))
	    value (nth 2 var) var (nth 1 var)))
  (plist-put org-tbl-calc-modes var value))

or, better, the code refactored to do not use this function?

Cheers,
Dan

[PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Daniele Nicolodi]

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

Hello,

attached there are a few patches reworking the code, fixing the bug, and
introducing a new mode flag to enable Calc's units simplification mode
as discussed in a recent thread on the mailing list.  I haven't updated
the documentation.  I can do it once we agree that this feature is a
good idea.

Cheers,
Dan


On 19/10/2020 17:38, Daniele Nicolodi wrote:
> Hello,
> 
> I am hacking org-table-eval-formula (see thread about monetary values in
> org-tables) which uses this inline function:
> 
> (defsubst org-table--set-calc-mode (var &optional value)
>   (if (stringp var)
>       (setq var (assoc var '(("D" calc-angle-mode deg)
> 			     ("R" calc-angle-mode rad)
> 			     ("F" calc-prefer-frac t)
> 			     ("S" calc-symbolic-mode t)))
> 	    value (nth 2 var) var (nth 1 var)))
>   (if (memq var org-tbl-calc-modes)
>       (setcar (cdr (memq var org-tbl-calc-modes)) value)
>     (cons var (cons value org-tbl-calc-modes)))
>   org-tbl-calc-modes)
> 
> which I am not able to understand or which is not correct.
> 
> The first (if ...) does some value substitutions, however, IIUC the
> second (if ...) sets a new value for an entry in the org-tbl-calc-modes
> plist if the entry is already present and builds a new plist with the
> entry prepended if the entry is not there. However, the original plist
> is returned and not the one with the new entry prepended.
> 
> It does not seem to be the intended behavior.
> 
> Shouldn't this be simply:
> 
> (defsubst org-table--set-calc-mode (var &optional value)
>   (if (stringp var)
>       (setq var (assoc var '(("D" calc-angle-mode deg)
> 			     ("R" calc-angle-mode rad)
> 			     ("F" calc-prefer-frac t)
> 			     ("S" calc-symbolic-mode t)))
> 	    value (nth 2 var) var (nth 1 var)))
>   (plist-put org-tbl-calc-modes var value))
> 
> or, better, the code refactored to do not use this function?
> 
> Cheers,
> Dan
> 


[-- Attachment #2: 0001-org-table-Fix-table-formula-mode-string-handling.patch --]
[-- Type: text/plain, Size: 2160 bytes --]

From c7434974897d932fe3acd182f06a98a61719e208 Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Tue, 20 Oct 2020 11:03:14 +0200
Subject: [PATCH 1/3] org-table: Fix table formula mode string handling

* lisp/org-table.el (org-table-eval-formula): Move mode lookup table
from org-table--set-calc-mode to here.

* lisp/org-table.el (org-table--set-calc-mode): Use plist-put instead
of the buggy open coded version.
---
 lisp/org-table.el | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 112b1e171..0790dc3ca 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -721,17 +721,8 @@ Field is restored even in case of abnormal exit."
 	 (org-table-goto-column ,column)
 	 (set-marker ,line nil)))))
 
-(defsubst org-table--set-calc-mode (var &optional value)
-  (if (stringp var)
-      (setq var (assoc var '(("D" calc-angle-mode deg)
-			     ("R" calc-angle-mode rad)
-			     ("F" calc-prefer-frac t)
-			     ("S" calc-symbolic-mode t)))
-	    value (nth 2 var) var (nth 1 var)))
-  (if (memq var org-tbl-calc-modes)
-      (setcar (cdr (memq var org-tbl-calc-modes)) value)
-    (cons var (cons value org-tbl-calc-modes)))
-  org-tbl-calc-modes)
+(defsubst org-table--set-calc-mode (var value)
+  (plist-put org-tbl-calc-modes var value))
 
 \f
 ;;; Predicates
@@ -2476,9 +2467,14 @@ location of point."
 		(setq keep-empty t
 		      fmt (replace-match "" t t fmt)))
 	    (while (string-match "[DRFS]" fmt)
-	      (setq org-tbl-calc-modes
-		    (org-table--set-calc-mode (match-string 0 fmt)))
-	      (setq fmt (replace-match "" t t fmt)))
+	      (let* ((c (string-to-char (match-string 0 fmt)))
+		     (mode (cdr (assoc c '((?D calc-angle-mode deg)
+					   (?R calc-angle-mode rad)
+					   (?F calc-prefer-frac t)
+					   (?S calc-symbolic-mode t))))))
+		(setq org-tbl-calc-modes (org-table--set-calc-mode
+					  (car mode) (cadr mode))
+		      fmt (replace-match "" t t fmt))))
 	    (unless (string-match "\\S-" fmt)
 	      (setq fmt nil))))
       (when (and (not suppress-const) org-table-formula-use-constants)
-- 
2.28.0


[-- Attachment #3: 0003-org-table-Add-mode-flag-to-enable-Calc-units-simplif.patch --]
[-- Type: text/plain, Size: 1301 bytes --]

From aad8eb548e7a7a7fde1908a9f9c66f980da10b56 Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Tue, 20 Oct 2020 15:22:02 +0200
Subject: [PATCH 3/3] org-table: Add mode flag to enable Calc units
 simplification mode

* org-table.el (org-table-eval-formula): Add the `u` mode flag to
enable Calc's units simplification mode.
---
 lisp/org-table.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 4baad2600..6b92656bd 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -2447,11 +2447,12 @@ location of point."
 		  (?e (org-table--set-calc-mode 'calc-float-format (list 'eng n)))))
 	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
-	    (while (string-match "\\([tTUNLEDRFS]\\)" fmt)
+	    (while (string-match "\\([tuTUNLEDRFS]\\)" fmt)
 	      (let ((c (string-to-char (match-string 1 fmt))))
 		(cl-case c
 		  (?t (setq duration t numbers t
 		      	    duration-output-format org-table-duration-custom-format))
+		  (?u (org-table--set-calc-mode 'calc-simplify-mode 'units))
 		  (?T (setq duration t numbers t duration-output-format nil))
 		  (?U (setq duration t numbers t duration-output-format 'hh:mm))
 		  (?N (setq numbers t))
-- 
2.28.0


[-- Attachment #4: 0002-org-table-Simplify-mode-string-parsing.patch --]
[-- Type: text/plain, Size: 5850 bytes --]

From fb8b62e5faabca2b6c6514e25bd306f7a5e8696f Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Tue, 20 Oct 2020 15:13:40 +0200
Subject: [PATCH 2/3] org-table: Simplify mode string parsing

* org-table.el (org-table-eval-formula): Simplify mode string parsing
and reduce scope of local variables.
---
 lisp/org-table.el | 98 +++++++++++++++++++++--------------------------
 1 file changed, 43 insertions(+), 55 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 0790dc3ca..4baad2600 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -722,7 +722,7 @@ Field is restored even in case of abnormal exit."
 	 (set-marker ,line nil)))))
 
 (defsubst org-table--set-calc-mode (var value)
-  (plist-put org-tbl-calc-modes var value))
+  (setq org-tbl-calc-modes (plist-put org-tbl-calc-modes var value)))
 
 \f
 ;;; Predicates
@@ -2427,54 +2427,42 @@ location of point."
 	   (org-tbl-calc-modes (copy-sequence org-calc-default-modes))
 	   (numbers nil)	   ; was a variable, now fixed default
 	   (keep-empty nil)
-	   n form form0 formrpl formrg bw fmt x ev orig c lispp literal
+	   form form0 formrpl formrg bw fmt ev orig lispp literal
 	   duration duration-output-format)
       ;; Parse the format string.  Since we have a lot of modes, this is
       ;; a lot of work.  However, I think calc still uses most of the time.
-      (if (string-match ";" formula)
-	  (let ((tmp (org-split-string formula ";")))
-	    (setq formula (car tmp)
-		  fmt (concat (cdr (assoc "%" org-table-local-parameters))
-			      (nth 1 tmp)))
+      (if (string-match "\\(.*\\);\\(.*\\)" formula)
+	  (progn
+	    (setq fmt (concat (match-string-no-properties 2 formula)
+			      (cdr (assoc "%" org-table-local-parameters)))
+		  formula (match-string-no-properties 1 formula))
 	    (while (string-match "\\([pnfse]\\)\\(-?[0-9]+\\)" fmt)
-	      (setq c (string-to-char (match-string 1 fmt))
-		    n (string-to-number (match-string 2 fmt)))
-	      (if (= c ?p)
-		  (setq org-tbl-calc-modes
-			(org-table--set-calc-mode 'calc-internal-prec n))
-		(setq org-tbl-calc-modes
-		      (org-table--set-calc-mode
-		       'calc-float-format
-		       (list (cdr (assoc c '((?n . float) (?f . fix)
-					     (?s . sci) (?e . eng))))
-			     n))))
+	      (let ((c (string-to-char (match-string 1 fmt)))
+		    (n (string-to-number (match-string 2 fmt))))
+		(cl-case c
+		  (?p (org-table--set-calc-mode 'calc-internal-prec n))
+		  (?n (org-table--set-calc-mode 'calc-float-format (list 'float n)))
+		  (?f (org-table--set-calc-mode 'calc-float-format (list 'fix n)))
+		  (?s (org-table--set-calc-mode 'calc-float-format (list 'sci n)))
+		  (?e (org-table--set-calc-mode 'calc-float-format (list 'eng n)))))
+	      ;; Remove matched flags from the mode string.
+	      (setq fmt (replace-match "" t t fmt)))
+	    (while (string-match "\\([tTUNLEDRFS]\\)" fmt)
+	      (let ((c (string-to-char (match-string 1 fmt))))
+		(cl-case c
+		  (?t (setq duration t numbers t
+		      	    duration-output-format org-table-duration-custom-format))
+		  (?T (setq duration t numbers t duration-output-format nil))
+		  (?U (setq duration t numbers t duration-output-format 'hh:mm))
+		  (?N (setq numbers t))
+		  (?L (setq literal t))
+		  (?E (setq keep-empty t))
+		  (?D (org-table--set-calc-mode 'calc-angle-mode 'deg))
+		  (?R (org-table--set-calc-mode 'calc-angle-mode 'rad))
+		  (?F (org-table--set-calc-mode 'calc-prefer-frac t))
+		  (?S (org-table--set-calc-mode 'calc-symbolic-mode t))))
+	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
-	    (if (string-match "[tTU]" fmt)
-		(let ((ff (match-string 0 fmt)))
-		  (setq duration t numbers t
-			duration-output-format
-			(cond ((equal ff "T") nil)
-			      ((equal ff "t") org-table-duration-custom-format)
-			      ((equal ff "U") 'hh:mm))
-			fmt (replace-match "" t t fmt))))
-	    (if (string-match "N" fmt)
-		(setq numbers t
-		      fmt (replace-match "" t t fmt)))
-	    (if (string-match "L" fmt)
-		(setq literal t
-		      fmt (replace-match "" t t fmt)))
-	    (if (string-match "E" fmt)
-		(setq keep-empty t
-		      fmt (replace-match "" t t fmt)))
-	    (while (string-match "[DRFS]" fmt)
-	      (let* ((c (string-to-char (match-string 0 fmt)))
-		     (mode (cdr (assoc c '((?D calc-angle-mode deg)
-					   (?R calc-angle-mode rad)
-					   (?F calc-prefer-frac t)
-					   (?S calc-symbolic-mode t))))))
-		(setq org-tbl-calc-modes (org-table--set-calc-mode
-					  (car mode) (cadr mode))
-		      fmt (replace-match "" t t fmt))))
 	    (unless (string-match "\\S-" fmt)
 	      (setq fmt nil))))
       (when (and (not suppress-const) org-table-formula-use-constants)
@@ -2575,17 +2563,17 @@ location of point."
 	(setq form0 form)
 	;; Insert the references to fields in same row
 	(while (string-match "\\$\\(\\([-+]\\)?[0-9]+\\)" form)
-	  (setq n (+ (string-to-number (match-string 1 form))
-		     (if (match-end 2) n0 0))
-		x (nth (1- (if (= n 0) n0 (max n 1))) fields)
-		formrpl (save-match-data
-			  (org-table-make-reference
-			   x keep-empty numbers lispp)))
-	  (when (or (not x)
-		    (save-match-data
-		      (string-match (regexp-quote formula) formrpl)))
-	    (user-error "Invalid field specifier \"%s\""
-			(match-string 0 form)))
+	  (let* ((n (+ (string-to-number (match-string 1 form))
+		       (if (match-end 2) n0 0)))
+		 (x (nth (1- (if (= n 0) n0 (max n 1))) fields)))
+	    (setq formrpl (save-match-data
+			    (org-table-make-reference
+			     x keep-empty numbers lispp)))
+	    (when (or (not x)
+		      (save-match-data
+			(string-match (regexp-quote formula) formrpl)))
+	      (user-error "Invalid field specifier \"%s\""
+			  (match-string 0 form))))
 	  (setq form (replace-match formrpl t t form)))
 
 	(if lispp
-- 
2.28.0

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Eric S Fraga]

Just to say that I have done a quick test with this and I really like
it.  Simple calculations with units of mol/s and mol/min work perfectly.

-- 
: Eric S Fraga via Emacs 28.0.50, Org release_9.4-61-ga88806.dirty

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Daniele Nicolodi]

On 20/10/2020 15:44, Eric S Fraga wrote:
> Just to say that I have done a quick test with this and I really like
> it.  Simple calculations with units of mol/s and mol/min work perfectly.

Thank you for testing Eric.

To cover the use case of monetary quantities discussed in the other
thread I would like to also add an `m?` (or M?) flag that can combine
the effect of the existing `f?` flag and the new `u` flag. But I don't
know if sparing one character is worth one character in the small flag
characters set.

Cheers,
Dan

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Eric S Fraga]

On Tuesday, 20 Oct 2020 at 16:00, Daniele Nicolodi wrote:
> To cover the use case of monetary quantities discussed in the other
> thread I would like to also add an `m?` (or M?) flag that can combine
> the effect of the existing `f?` flag and the new `u` flag. But I don't
> know if sparing one character is worth one character in the small flag
> characters set.

Well, I personally am perfectly fine with uf3 instead of m3 (say) as the
former has a clear intent.

But, as you'll have seen by my followup message, there is something
strange going on with u and f combined.  But I'm still playing.
-- 
: Eric S Fraga via Emacs 28.0.50, Org release_9.4-61-ga88806.dirty

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Eric S Fraga]

Hello again,

Following up on myself.  I'm seeing some strange behaviour although unit
calculations are working nicely.  For instance, this table:

#+begin_src org
  | stream   | a            | b            | c            | total        |   x_a |   x_b |   x_c |
  |          | <l>          | <l>          | <l>          | <l>          |   <r> |   <r> |   <r> |
  |----------+--------------+--------------+--------------+--------------+-------+-------+-------|
  | feed     | 1.05 mol/s   | 1.05 mol/s   |              | 2.10 mol / s | 0.500 | 0.500 |     0 |
  | effluent | 0.74 mol / s | 0.74 mol / s | 0.32 mol / s | 1.80 mol / s | 0.411 | 0.411 | 0.178 |
  ,#+TBLFM: $5=vsum($2..$4);uf2::$6=$2/$5;uf3::$7=$3/$5;uf3::$8=$4/$5;uf3::@4$2=(1-0.3)*@-1;uf2::@4$3=(1-0.3)*@-1;uf2::@4$4=@-1+0.3*@-1$-1;uf2
#+end_src

does not seem to pay attention to the f3 mode in the last column, first
data row.

I've also seen some (difficult to replicate) problem with column widths
where the columns are much wider than the expected.  I'll keep playing
to see if I can isolate the column width behaviour.

-- 
: Eric S Fraga via Emacs 28.0.50, Org release_9.4-61-ga88806.dirty

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Daniele Nicolodi]

On 20/10/2020 16:19, Eric S Fraga wrote:
> Hello again,
> 
> Following up on myself.  I'm seeing some strange behaviour although unit
> calculations are working nicely.  For instance, this table:
> 
> #+begin_src org
>   | stream   | a            | b            | c            | total        |   x_a |   x_b |   x_c |
>   |          | <l>          | <l>          | <l>          | <l>          |   <r> |   <r> |   <r> |
>   |----------+--------------+--------------+--------------+--------------+-------+-------+-------|
>   | feed     | 1.05 mol/s   | 1.05 mol/s   |              | 2.10 mol / s | 0.500 | 0.500 |     0 |
>   | effluent | 0.74 mol / s | 0.74 mol / s | 0.32 mol / s | 1.80 mol / s | 0.411 | 0.411 | 0.178 |
>   ,#+TBLFM: $5=vsum($2..$4);uf2::$6=$2/$5;uf3::$7=$3/$5;uf3::$8=$4/$5;uf3::@4$2=(1-0.3)*@-1;uf2::@4$3=(1-0.3)*@-1;uf2::@4$4=@-1+0.3*@-1$-1;uf2
> #+end_src
> 
> does not seem to pay attention to the f3 mode in the last column, first
> data row.

It is something related to how Calc computes the result. The f2 mode
specifies the formatting for floating point values, however it seems
that Calc treats the zero (from the missing value in the fourth column)
divided by a float (from the value in the fifth column) as an integer
and not as a float. This may be because the org substitutes a "0" for
the missing value, thus an integer. Still, I am not sure dividing an
integer by a float should result in an integer (I guess zero is special
cased here).

If you change the formula for that field to:

  #+TBLFM: $8=$4*1.0/$5;uf3

to force the $4 field to be evaluated as a float (there are other ways
to get the same effect) you get the expected result (I think).

> I've also seen some (difficult to replicate) problem with column widths
> where the columns are much wider than the expected.  I'll keep playing
> to see if I can isolate the column width behaviour.

I haven't touched any code dealing with columns width (I believe).

Cheers,
Dan

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Daniele Nicolodi]

On 20/10/2020 16:32, Daniele Nicolodi wrote:
> On 20/10/2020 16:19, Eric S Fraga wrote:
>> Hello again,
>>
>> Following up on myself.  I'm seeing some strange behaviour although unit
>> calculations are working nicely.  For instance, this table:
>>
>> #+begin_src org
>>   | stream   | a            | b            | c            | total        |   x_a |   x_b |   x_c |
>>   |          | <l>          | <l>          | <l>          | <l>          |   <r> |   <r> |   <r> |
>>   |----------+--------------+--------------+--------------+--------------+-------+-------+-------|
>>   | feed     | 1.05 mol/s   | 1.05 mol/s   |              | 2.10 mol / s | 0.500 | 0.500 |     0 |
>>   | effluent | 0.74 mol / s | 0.74 mol / s | 0.32 mol / s | 1.80 mol / s | 0.411 | 0.411 | 0.178 |
>>   ,#+TBLFM: $5=vsum($2..$4);uf2::$6=$2/$5;uf3::$7=$3/$5;uf3::$8=$4/$5;uf3::@4$2=(1-0.3)*@-1;uf2::@4$3=(1-0.3)*@-1;uf2::@4$4=@-1+0.3*@-1$-1;uf2
>> #+end_src
>>
>> does not seem to pay attention to the f3 mode in the last column, first
>> data row.
> 
> It is something related to how Calc computes the result. The f2 mode
> specifies the formatting for floating point values, however it seems
> that Calc treats the zero (from the missing value in the fourth column)
> divided by a float (from the value in the fifth column) as an integer
> and not as a float. This may be because the org substitutes a "0" for
> the missing value, thus an integer. Still, I am not sure dividing an
> integer by a float should result in an integer (I guess zero is special
> cased here).
> 
> If you change the formula for that field to:
> 
>   #+TBLFM: $8=$4*1.0/$5;uf3
> 
> to force the $4 field to be evaluated as a float (there are other ways
> to get the same effect) you get the expected result (I think).

There are other funny Calc behaviors: if an expression results in a
number with an unit where the numerical part is exactly 1, the printed
result looses the numerical part and only the units are printed.

Cheers,
Dan

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Eric S Fraga]

On Tuesday, 20 Oct 2020 at 16:53, Daniele Nicolodi wrote:
> There are other funny Calc behaviors: if an expression results in a
> number with an unit where the numerical part is exactly 1, the printed
> result looses the numerical part and only the units are printed.

Yes, this I'm used to (and it is *annoying* sometimes).  But given what
calc brings to the party, I can put up with small things like this!

Thanks again,
eric

-- 
: Eric S Fraga via Emacs 28.0.50, Org release_9.4-61-ga88806.dirty

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Eric S Fraga]

On Tuesday, 20 Oct 2020 at 16:32, Daniele Nicolodi wrote:
> This may be because the org substitutes a "0" for the missing value,
> thus an integer. 

This makes sense.  Thank you.

-- 
: Eric S Fraga via Emacs 28.0.50, Org release_9.4-61-ga88806.dirty

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Daniele Nicolodi]

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

Hello,

after working on this I realized that the org-tbl-calc-modes variables
is used only locally despite being declare globally. Maybe a remnant
from pre-lexical-binding times. Attached is a patch (on top of the
others one in this thread) that simplifies things a little.

Cheers,
Dan


On 20/10/2020 15:30, Daniele Nicolodi wrote:
> Hello,
> 
> attached there are a few patches reworking the code, fixing the bug, and
> introducing a new mode flag to enable Calc's units simplification mode
> as discussed in a recent thread on the mailing list.  I haven't updated
> the documentation.  I can do it once we agree that this feature is a
> good idea.
> 
> Cheers,
> Dan
> 
> 
> On 19/10/2020 17:38, Daniele Nicolodi wrote:
>> Hello,
>>
>> I am hacking org-table-eval-formula (see thread about monetary values in
>> org-tables) which uses this inline function:
>>
>> (defsubst org-table--set-calc-mode (var &optional value)
>>   (if (stringp var)
>>       (setq var (assoc var '(("D" calc-angle-mode deg)
>> 			     ("R" calc-angle-mode rad)
>> 			     ("F" calc-prefer-frac t)
>> 			     ("S" calc-symbolic-mode t)))
>> 	    value (nth 2 var) var (nth 1 var)))
>>   (if (memq var org-tbl-calc-modes)
>>       (setcar (cdr (memq var org-tbl-calc-modes)) value)
>>     (cons var (cons value org-tbl-calc-modes)))
>>   org-tbl-calc-modes)
>>
>> which I am not able to understand or which is not correct.
>>
>> The first (if ...) does some value substitutions, however, IIUC the
>> second (if ...) sets a new value for an entry in the org-tbl-calc-modes
>> plist if the entry is already present and builds a new plist with the
>> entry prepended if the entry is not there. However, the original plist
>> is returned and not the one with the new entry prepended.
>>
>> It does not seem to be the intended behavior.
>>
>> Shouldn't this be simply:
>>
>> (defsubst org-table--set-calc-mode (var &optional value)
>>   (if (stringp var)
>>       (setq var (assoc var '(("D" calc-angle-mode deg)
>> 			     ("R" calc-angle-mode rad)
>> 			     ("F" calc-prefer-frac t)
>> 			     ("S" calc-symbolic-mode t)))
>> 	    value (nth 2 var) var (nth 1 var)))
>>   (plist-put org-tbl-calc-modes var value))
>>
>> or, better, the code refactored to do not use this function?
>>
>> Cheers,
>> Dan
>>
> 


[-- Attachment #2: 0004-org-table-Remove-unused-org-tbl-calc-modes-variable.patch --]
[-- Type: text/plain, Size: 4374 bytes --]

From 2d4521a032ec3e4174c97b2b2e9a08491e9870fb Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Wed, 21 Oct 2020 17:47:15 +0200
Subject: [PATCH 4/4] org-table: Remove unused org-tbl-calc-modes variable

* org-table.el (org-tbl-calc-modes): Remove the variable declaration
as the varialble is only used as a local variable in `org-table-eval-formula'.

* org-table.el (org-table--set-calc-mode): Drop convenience macro.

* org-table.el (org-table-eval-formula): Rename `org-tbl-calc-modes`
local variable without the org-table prefix and usr the gained screen
real estate to avoid indirection through covenience macro.
---
 lisp/org-table.el | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 6b92656bd..1651decd3 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -676,8 +676,6 @@ Will be filled automatically during use.")
     ("_" . "Names for values in row below this one.")
     ("^" . "Names for values in row above this one.")))
 
-(defvar org-tbl-calc-modes nil)
-
 (defvar org-pos nil)
 
 \f
@@ -721,9 +719,6 @@ Field is restored even in case of abnormal exit."
 	 (org-table-goto-column ,column)
 	 (set-marker ,line nil)))))
 
-(defsubst org-table--set-calc-mode (var value)
-  (setq org-tbl-calc-modes (plist-put org-tbl-calc-modes var value)))
-
 \f
 ;;; Predicates
 
@@ -2424,7 +2419,7 @@ location of point."
 			equation
 		      (org-table-get-formula equation (equal arg '(4)))))
 	   (n0 (org-table-current-column))
-	   (org-tbl-calc-modes (copy-sequence org-calc-default-modes))
+	   (calc-modes (copy-sequence org-calc-default-modes))
 	   (numbers nil)	   ; was a variable, now fixed default
 	   (keep-empty nil)
 	   form form0 formrpl formrg bw fmt ev orig lispp literal
@@ -2440,11 +2435,11 @@ location of point."
 	      (let ((c (string-to-char (match-string 1 fmt)))
 		    (n (string-to-number (match-string 2 fmt))))
 		(cl-case c
-		  (?p (org-table--set-calc-mode 'calc-internal-prec n))
-		  (?n (org-table--set-calc-mode 'calc-float-format (list 'float n)))
-		  (?f (org-table--set-calc-mode 'calc-float-format (list 'fix n)))
-		  (?s (org-table--set-calc-mode 'calc-float-format (list 'sci n)))
-		  (?e (org-table--set-calc-mode 'calc-float-format (list 'eng n)))))
+		  (?p (setf (cl-getf calc-modes 'calc-internal-prec) n))
+		  (?n (setf (cl-getf calc-modes 'calc-float-format) (list 'float n)))
+		  (?f (setf (cl-getf calc-modes 'calc-float-format) (list 'fix n)))
+		  (?s (setf (cl-getf calc-modes 'calc-float-format) (list 'sci n)))
+		  (?e (setf (cl-getf calc-modes 'calc-float-format) (list 'eng n)))))
 	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
 	    (while (string-match "\\([tuTUNLEDRFS]\\)" fmt)
@@ -2452,16 +2447,16 @@ location of point."
 		(cl-case c
 		  (?t (setq duration t numbers t
 		      	    duration-output-format org-table-duration-custom-format))
-		  (?u (org-table--set-calc-mode 'calc-simplify-mode 'units))
+		  (?u (setf (cl-getf calc-modes 'calc-simplify-mode) 'units))
 		  (?T (setq duration t numbers t duration-output-format nil))
 		  (?U (setq duration t numbers t duration-output-format 'hh:mm))
 		  (?N (setq numbers t))
 		  (?L (setq literal t))
 		  (?E (setq keep-empty t))
-		  (?D (org-table--set-calc-mode 'calc-angle-mode 'deg))
-		  (?R (org-table--set-calc-mode 'calc-angle-mode 'rad))
-		  (?F (org-table--set-calc-mode 'calc-prefer-frac t))
-		  (?S (org-table--set-calc-mode 'calc-symbolic-mode t))))
+		  (?D (setf (cl-getf calc-modes 'calc-angle-mode) 'deg))
+		  (?R (setf (cl-getf calc-modes 'calc-angle-mode) 'rad))
+		  (?F (setf (cl-getf calc-modes 'calc-prefer-frac) t))
+		  (?S (setf (cl-getf calc-modes 'calc-symbolic-mode) t))))
 	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
 	    (unless (string-match "\\S-" fmt)
@@ -2606,7 +2601,7 @@ location of point."
 
 	  (setq ev (if (and duration (string-match "^[0-9]+:[0-9]+\\(?::[0-9]+\\)?$" form))
 		       form
-		     (calc-eval (cons form org-tbl-calc-modes)
+		     (calc-eval (cons form calc-modes)
 				(when (and (not keep-empty) numbers) 'num)))
 		ev (if duration (org-table-time-seconds-to-string
 				 (if (string-match "^[0-9]+:[0-9]+\\(?::[0-9]+\\)?$" ev)
-- 
2.28.0