Scan of broken conditional forms

Scan of broken conditional forms

[Mattias Engdegård]

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

Out of curiosity, I scanned the Emacs sources for useless conditional forms, like (if C X X) where both branches are the same, and found a few. Some look like unfinished code, but a several appear to be clear bugs. (I'm guilty of the one in autorevert.el, now fixed.)

In particular, the code in mml-smime.el looks like it fails to return failure from an unsuccessful encryption attempt. That can't be good.


[-- Attachment #2: iffy.log --]
[-- Type: application/octet-stream, Size: 791 bytes --]

;;; -*- compilation -*-

lisp/international/titdic-cnv.el:740: (if CONDITION X X)
lisp/international/titdic-cnv.el:741: (if CONDITION X X)
lisp/autorevert.el:736: (unless CONDITION)
lisp/emacs-lisp/tabulated-list.el:549: (when CONDITION)
lisp/filesets.el:1651: (if CONDITION CONST1 CONST2) ...
lisp/cedet/ede/cpp-root.el:490: (if CONDITION CONST) ...
lisp/cedet/ede/pconf.el:55: (when CONDITION)
lisp/cedet/srecode/semantic.el:203: (when CONDITION CONST) ...
lisp/gnus/gnus-cloud.el:243: (when CONDITION)
lisp/gnus/mml-smime.el:157: (if CONDITION (progn A t) A nil)
lisp/net/nsm.el:297: (when CONDITION)
lisp/net/tramp-sudoedit.el:509: (when CONDITION)
lisp/org/ox-odt.el:942: (if CONDITION X X)
lisp/org/org-agenda.el:8984: (when CONDITION)
lisp/textmodes/table.el:3210: (unless CONDITION)

Re: Scan of broken conditional forms

[Michael Albinus]

Mattias Engdegård <mattiase@acm.org> writes:

Hi Mattias,

> Out of curiosity, I scanned the Emacs sources for useless conditional
> forms, like (if C X X) where both branches are the same, and found a
> few. Some look like unfinished code, but a several appear to be clear
> bugs. (I'm guilty of the one in autorevert.el, now fixed.)
>
> lisp/net/tramp-sudoedit.el:509: (when CONDITION)

This is indeed a bug, thanks for reporting. I've fixed it in the
emacs-27 branch.

Best regards, Michael.

Re: Scan of broken conditional forms

[Paul Eggert]

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

Thanks for doing that checking.

On 1/4/20 4:37 AM, Mattias Engdegård wrote:

> the code in mml-smime.el looks like it fails to return failure from an unsuccessful encryption attempt

smime-encrypt-buffer always returns nil (it signals an error on failure), so
that code happens to work though it is too complicated.

I fixed that issue along with the others (other than the two that you and
Michael already fixed) by installing the attached into the emacs-27 branch.

[-- Attachment #2: 0001-Fix-some-broken-conditional-forms.patch --]
[-- Type: text/x-patch, Size: 10851 bytes --]

From f95a2b83014a810d508448473b20186d55485efd Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 4 Jan 2020 11:17:12 -0800
Subject: [PATCH] Fix some broken conditional forms
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem reported by Mattias Engdegård in:
https://lists.gnu.org/r/emacs-devel/2020-01/msg00088.html
* lisp/cedet/ede/cpp-root.el (ede-create-lots-of-projects-under-dir):
Remove this quick hack, which didn’t do anything anyway.
* lisp/cedet/ede/pconf.el (ede-proj-configure-test-required-file):
* lisp/emacs-lisp/tabulated-list.el (tabulated-list-print-col):
* lisp/net/nsm.el (nsm-check-tls-connection):
Use ‘when’ rather than bypassing it.  This doesn’t affect behavior
and is better style.
* lisp/cedet/srecode/semantic.el (srecode-semantic-handle-:tag):
Fix typo that suppressed an error.
* lisp/filesets.el (filesets-run-cmd): Fix typo that mishandled spacing.
* lisp/gnus/gnus-cloud.el (gnus-cloud-update-newsrc-data):
Fix typo that caused “GROUP has older different info in the cloud
as of DATE, update it here?” prompt result to always be treated as
“yes”.
* lisp/gnus/mml-smime.el (mml-smime-openssl-encrypt): Simplify,
since smime-encrypt-buffer signals error on failure.
* lisp/international/titdic-cnv.el (tsang-quick-converter): Simplify.
The conversion of this file to utf-8-emacs in
2019-01-08T02:18:40Z!monnier@iro.umontreal.ca removed the
distinction between Big5 and CNS fulltitles in the generated docstring.
* lisp/org/org-agenda.el (org-agenda-show-and-scroll-up):
* lisp/textmodes/table.el (table--generate-source-cell-contents):
Simplify by removing useless code.
* lisp/org/ox-odt.el (org-odt--format-timestamp): Fix typo that
always output time-of-day even when the timestamp lacked it.
---
 lisp/cedet/ede/cpp-root.el        | 15 ---------------
 lisp/cedet/ede/pconf.el           |  5 +++--
 lisp/cedet/srecode/semantic.el    |  2 +-
 lisp/emacs-lisp/tabulated-list.el |  8 ++++----
 lisp/filesets.el                  |  2 +-
 lisp/gnus/gnus-cloud.el           |  8 ++++----
 lisp/gnus/mml-smime.el            | 11 +++--------
 lisp/international/titdic-cnv.el  |  3 +--
 lisp/net/nsm.el                   |  6 +++---
 lisp/org/org-agenda.el            |  1 -
 lisp/org/ox-odt.el                |  2 +-
 lisp/textmodes/table.el           |  6 +-----
 12 files changed, 22 insertions(+), 47 deletions(-)

diff --git a/lisp/cedet/ede/cpp-root.el b/lisp/cedet/ede/cpp-root.el
index ee8aa5db1b..f0dbccb7fc 100644
--- a/lisp/cedet/ede/cpp-root.el
+++ b/lisp/cedet/ede/cpp-root.el
@@ -478,21 +478,6 @@ ede-cpp-root-project
   "Don't rescan this project from the sources."
   (message "cpp-root has nothing to rescan."))
 
-;;; Quick Hack
-(defun ede-create-lots-of-projects-under-dir (dir projfile &rest attributes)
-  "Create a bunch of projects under directory DIR.
-PROJFILE is a file name sans directory that indicates a subdirectory
-is a project directory.
-Generic ATTRIBUTES, such as :include-path can be added.
-Note: This needs some work."
-  (let ((files (directory-files dir t)))
-    (dolist (F files)
-      (if (file-exists-p (expand-file-name projfile F))
-	  `(ede-cpp-root-project (file-name-nondirectory F)
-				 :name (file-name-nondirectory F)
-				 :file (expand-file-name projfile F)
-				 attributes)))))
-
 (provide 'ede/cpp-root)
 
 ;; Local variables:
diff --git a/lisp/cedet/ede/pconf.el b/lisp/cedet/ede/pconf.el
index 63fb62b5a5..b85b397af2 100644
--- a/lisp/cedet/ede/pconf.el
+++ b/lisp/cedet/ede/pconf.el
@@ -56,8 +56,9 @@ ede-pconf-create-file-query
 	    (and (eq ede-pconf-create-file-query 'ask)
 		 (not (eq ede-pconf-create-file-query 'never))
 		 (not (y-or-n-p
-		       (format "I had to create the %s file for you.  Ok? " file)))
-		 (error "Quit")))))))
+		       (format "I had to create the %s file for you.  Ok? "
+			       file))))
+	  (error "Quit"))))))
 
 
 (cl-defmethod ede-proj-configure-synchronize ((this ede-proj-project))
diff --git a/lisp/cedet/srecode/semantic.el b/lisp/cedet/srecode/semantic.el
index 26c14892ef..5b2dd03474 100644
--- a/lisp/cedet/srecode/semantic.el
+++ b/lisp/cedet/srecode/semantic.el
@@ -201,7 +201,7 @@ srecode-semantic-handle-:tag
   (let ((tag (or srecode-semantic-selected-tag
 		 (srecode-semantic-tag-from-kill-ring))))
     (when (not tag)
-      "No tag for current template.  Use the semantic kill-ring.")
+      (error "No tag for current template.  Use the semantic kill-ring."))
     (srecode-semantic-apply-tag-to-dict
      (srecode-semantic-tag (semantic-tag-name tag)
 			   :prime tag)
diff --git a/lisp/emacs-lisp/tabulated-list.el b/lisp/emacs-lisp/tabulated-list.el
index 501cc3a29e..b13f609f88 100644
--- a/lisp/emacs-lisp/tabulated-list.el
+++ b/lisp/emacs-lisp/tabulated-list.el
@@ -547,10 +547,10 @@ tabulated-list-print-col
     ;; Don't truncate to `width' if the next column is align-right
     ;; and has some space left, truncate to `available-space' instead.
     (when (and not-last-col
-               (> label-width available-space)
-               (setq label (truncate-string-to-width
-                            label available-space nil nil t t)
-                     label-width available-space)))
+	       (> label-width available-space))
+      (setq label (truncate-string-to-width
+		   label available-space nil nil t t)
+	    label-width available-space))
     (setq label (bidi-string-mark-left-to-right label))
     (when (and right-align (> width label-width))
       (let ((shift (- width label-width)))
diff --git a/lisp/filesets.el b/lisp/filesets.el
index 9834bcf058..1ec0d24b53 100644
--- a/lisp/filesets.el
+++ b/lisp/filesets.el
@@ -1645,10 +1645,10 @@ filesets-run-cmd
 				      (dolist (this args txt)
 					(setq txt
 					      (concat txt
+						      (if (equal txt "") "" " ")
 						      (filesets-run-cmd--repl-fn
 						       this
 						       (lambda (this)
-							 (if (equal txt "") "" " ")
 							 (format "%s" this))))))))
 				   (cmd (concat fn " " args)))
 			      (filesets-cmd-show-result
diff --git a/lisp/gnus/gnus-cloud.el b/lisp/gnus/gnus-cloud.el
index cecfaef2f4..4d8764bacc 100644
--- a/lisp/gnus/gnus-cloud.el
+++ b/lisp/gnus/gnus-cloud.el
@@ -243,10 +243,10 @@ gnus-cloud-update-newsrc-data
                 (when (or (not gnus-cloud-interactive)
                           (gnus-y-or-n-p
                            (format "%s has older different info in the cloud as of %s, update it here? "
-                                   group date))))
-                (gnus-message 2 "Installing cloud update of group %s" group)
-                (gnus-set-info group contents)
-                (gnus-group-update-group group)))
+				   group date)))
+		  (gnus-message 2 "Installing cloud update of group %s" group)
+		  (gnus-set-info group contents)
+		  (gnus-group-update-group group))))
           (gnus-error 1 "Sorry, group %s is not subscribed" group))
       (gnus-error 1 "Sorry, could not update newsrc for group %s (invalid data %S)"
                   group elem))))
diff --git a/lisp/gnus/mml-smime.el b/lisp/gnus/mml-smime.el
index 3cc463d5d4..4754f37a2d 100644
--- a/lisp/gnus/mml-smime.el
+++ b/lisp/gnus/mml-smime.el
@@ -154,14 +154,9 @@ mml-smime-openssl-encrypt
 	  (write-region (point-min) (point-max) file))
 	(push file certfiles)
 	(push file tmpfiles)))
-    (if (smime-encrypt-buffer certfiles)
-	(progn
-	  (while (setq tmp (pop tmpfiles))
-	    (delete-file tmp))
-	  t)
-      (while (setq tmp (pop tmpfiles))
-	(delete-file tmp))
-      nil))
+    (smime-encrypt-buffer certfiles)
+    (while (setq tmp (pop tmpfiles))
+      (delete-file tmp)))
   (goto-char (point-max)))
 
 (defvar gnus-extract-address-components)
diff --git a/lisp/international/titdic-cnv.el b/lisp/international/titdic-cnv.el
index 2a80d75fe7..e95e399eda 100644
--- a/lisp/international/titdic-cnv.el
+++ b/lisp/international/titdic-cnv.el
@@ -737,8 +737,7 @@ quail-misc-package-ext-info
 ;; method is for inputting CNS characters.
 
 (defun tsang-quick-converter (dicbuf tsang-p big5-p)
-  (let ((fulltitle (if tsang-p (if big5-p "倉頡" "倉頡")
-		     (if big5-p "簡易" "簡易")))
+  (let ((fulltitle (if tsang-p "倉頡" "簡易"))
 	dic)
     (goto-char (point-max))
     (if big5-p
diff --git a/lisp/net/nsm.el b/lisp/net/nsm.el
index e94947bc7f..1b0f04e5a1 100644
--- a/lisp/net/nsm.el
+++ b/lisp/net/nsm.el
@@ -311,9 +311,9 @@ nsm-check-tls-connection
                                                   (map-values results)
                                                   "\n")
                                                  "\n")
-                                                "\n* ")))))
-                 (delete-process process)
-                 (setq process nil)))
+						"\n* "))))))
+	(delete-process process)
+	(setq process nil))
       (run-hook-with-args 'nsm-tls-post-check-functions
                           host port status settings results)))
   process)
diff --git a/lisp/org/org-agenda.el b/lisp/org/org-agenda.el
index 4f89ea5450..f45e47fb59 100644
--- a/lisp/org/org-agenda.el
+++ b/lisp/org/org-agenda.el
@@ -8981,7 +8981,6 @@ org-agenda-show-and-scroll-up
 	 (narrow-to-region (org-entry-beginning-position)
 			   (org-entry-end-position))
 	 (org-show-all '(drawers))))
-      (when arg )
       (setq org-agenda-show-window (selected-window)))
     (select-window win)))
 
diff --git a/lisp/org/ox-odt.el b/lisp/org/ox-odt.el
index 51cb42a49a..a1486318a7 100644
--- a/lisp/org/ox-odt.el
+++ b/lisp/org/ox-odt.el
@@ -940,7 +940,7 @@ org-odt--format-timestamp
 	 (has-time-p (or (not timestamp)
 			 (org-timestamp-has-time-p timestamp)))
 	 (iso-date (let ((format (if has-time-p "%Y-%m-%dT%H:%M:%S"
-				   "%Y-%m-%dT%H:%M:%S")))
+				   "%Y-%m-%d")))
 		     (funcall format-timestamp timestamp format end))))
     (if iso-date-p iso-date
       (let* ((style (if has-time-p "OrgDate2" "OrgDate1"))
diff --git a/lisp/textmodes/table.el b/lisp/textmodes/table.el
index 4482e7d4d2..a33e9266b4 100644
--- a/lisp/textmodes/table.el
+++ b/lisp/textmodes/table.el
@@ -3206,11 +3206,7 @@ table--generate-source-cell-contents
 	  (while (and (re-search-forward "$" nil t)
 		      (not (eobp)))
 	    (insert "<br />")
-	    (forward-char 1)))
-	(unless (and table-html-delegate-spacing-to-user-agent
-		     (progn
-		       (goto-char (point-min))
-		       (looking-at "\\s *\\'")))))
+	    (forward-char 1))))
        ((eq language 'cals)
 	(table--remove-eol-spaces (point-min) (point-max))
 	(if (re-search-forward "\\s +\\'" nil t)
-- 
2.17.1

Re: Scan of broken conditional forms

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 4 Jan 2020 11:23:29 -0800
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> Thanks for doing that checking.
> 
> On 1/4/20 4:37 AM, Mattias Engdegård wrote:
> 
> > the code in mml-smime.el looks like it fails to return failure from an unsuccessful encryption attempt
> 
> smime-encrypt-buffer always returns nil (it signals an error on failure), so
> that code happens to work though it is too complicated.
> 
> I fixed that issue along with the others (other than the two that you and
> Michael already fixed) by installing the attached into the emacs-27 branch.

Why are we making non-essential changes on the release branch?

Re: Scan of broken conditional forms

[Paul Eggert]

On 1/4/20 11:39 AM, Eli Zaretskii wrote:
> Why are we making non-essential changes on the release branch?

That particular change was a mistake. I thought I was installing into master,
but installed into emacs-27. As I don't think it matters much either way I left
it alone. If you prefer that I revert the emacs-27 change and install it into
master please let me know.

Re: Scan of broken conditional forms

[Mattias Engdegård]

4 jan. 2020 kl. 20.23 skrev Paul Eggert <eggert@cs.ucla.edu>:

> smime-encrypt-buffer always returns nil (it signals an error on failure), so
> that code happens to work though it is too complicated.

Oh, right --- I was led astray by the code that looked at the return value.

> I fixed that issue along with the others (other than the two that you and
> Michael already fixed) by installing the attached into the emacs-27 branch.

Thank you, and the changes seem fine as far as I can tell.

One point though: Doesn't ede-proj-configure-test-required-file strike you as a bit backwards? It creates a file, and then asks for permission afterwards (easier to obtain forgiveness...), despite the assurances in the docstring of ede-pconf-create-file-query. The actual condition logic is bonkers, too (the 'never' value has the same effect as nil).

Re: Scan of broken conditional forms

[Paul Eggert]

On 1/4/20 2:04 PM, Mattias Engdegård wrote:
> Doesn't ede-proj-configure-test-required-file strike you as a bit backwards?

I doubt whether anyone uses it. Feel free to add a comment saying that it's
surely busted.

Re: Scan of broken conditional forms

[Eli Zaretskii]

> Cc: mattiase@acm.org, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 4 Jan 2020 13:40:50 -0800
> 
> On 1/4/20 11:39 AM, Eli Zaretskii wrote:
> > Why are we making non-essential changes on the release branch?
> 
> That particular change was a mistake. I thought I was installing into master,
> but installed into emacs-27. As I don't think it matters much either way I left
> it alone. If you prefer that I revert the emacs-27 change and install it into
> master please let me know.

Yes, let's install this only on master, please.  Some of the changes
are non-trivial enough to make me uneasy.

Btw, the change in titdic-cnv.el, by itself, makes no sense: that
function is full of other duplicate identical strings from which it
selects using the big5-p flag, just look at the code starting from
this:

    (let ((punctuation '((";" "；﹔，、﹐﹑" "；﹔，、﹐﹑")

and ending with

		      (if big5-p (nth 1 elt) (nth 2 elt))))))

When Stefan recoded this file, he left that code intact, which now
makes no sense at all.  We should probably propertize the strings with
the 2 corresponding charset properties, something that before the
recoding happened automagically (because ISO-2022 records the charset
in the encoding), and which was the whole purpose of this function.

Thanks.

Re: Scan of broken conditional forms

[Paul Eggert]

On 1/5/20 7:45 AM, Eli Zaretskii wrote:

> let's install this only on master, please.

OK, I did that.
> Btw, the change in titdic-cnv.el, by itself, makes no sense ...
> When Stefan recoded this file, he left that code intact, which now
> makes no sense at all.  We should probably propertize the strings with
> the 2 corresponding charset properties, something that before the
> recoding happened automagically (because ISO-2022 records the charset
> in the encoding), and which was the whole purpose of this function.

I worked around the problem by converting titdic-cnv.el back to iso-2022-7bit on
master, as this conversion was simple. Stefan (or anybody) can look into this
later if they want to do it in a better way.

Re: Scan of broken conditional forms

[Stefan Monnier]

> I worked around the problem by converting titdic-cnv.el back to iso-2022-7bit on
> master, as this conversion was simple. Stefan (or anybody) can look into this
> later if they want to do it in a better way.

Not without an actual test case.


        Stefan

Re: Scan of broken conditional forms

[Bastien]

Hi Mattias,

> Out of curiosity, I scanned the Emacs sources for useless conditional
> forms, like (if C X X) where both branches are the same, and found a
> few. Some look like unfinished code, but a several appear to be clear
> bugs. (I'm guilty of the one in autorevert.el, now fixed.)

very useful, thanks.  I fixed the two occurrences related to Org.

Best,

-- 
 Bastien

Broken `if big5-p` code in titdic-cnv.el (was: Scan of broken conditional forms)

[Stefan Monnier]

[ This dates back to Jan 2020.

  To recap we have in titdic-cnv.el code like:

    (defun tsang-quick-converter (dicbuf tsang-p big5-p)
      (let ((fulltitle (if tsang-p (if big5-p "倉頡" "倉頡")
                         (if big5-p "簡易" "簡易")))

  where the `if big5-p` tests appear to do nothing.  It turns out that
  those two strings have the same unicode chars but because the file is
  encoded using iso-2022-jp they have a different `charset` property
  applied to them which Emacs can use to render them differently.
  
  When we bumped into this code, tho, the file has been converted to
  `utf-8` (by yours truly) so that the nuance had been lost.
  Paul reverted this part of my change to recover the subtle rendering.  ]

Paul Eggert [2020-01-05 12:48:29] wrote:
> On 1/5/20 7:45 AM, Eli Zaretskii wrote:
>> let's install this only on master, please.
> OK, I did that.
>> Btw, the change in titdic-cnv.el, by itself, makes no sense ...
>> When Stefan recoded this file, he left that code intact, which now
>> makes no sense at all.  We should probably propertize the strings with
>> the 2 corresponding charset properties, something that before the
>> recoding happened automagically (because ISO-2022 records the charset
>> in the encoding), and which was the whole purpose of this function.
> I worked around the problem by converting titdic-cnv.el back to
> iso-2022-7bit on master, as this conversion was simple.  Stefan (or
> anybody) can look into this later if they want to do it in
> a better way.

I just looked into it and I still can't see what's wrong with using
utf-8 here.  AFAICT those `if big5-p` tests have been doing nothing ever
since Emacs's internal encoding was changed to be based on Unicode
(i.e. Emacs-23).

While it's true that using the iso-2022-jp encoding on the file does
allow Emacs to render the two strings differently, this only applies to
the source file.  The .elc files all use `utf-8-emacs` encoding anyway,
so that info is lost.  And the difference is even lost before we write
the .elc file because when Emacs byte-compiles that code the
byte-compiler considers those two strings as "equal" and emits only one
string in the byte-code (so the two branches return `eq` strings).

So, I think using `iso-2022-jp` is a bad idea here: it gives the
illusion that the two branches are different where they really aren't.
If we do want to recover the difference (the one we presumably lost in
Emacs-23), we need to make those two branches return
properly-propertized strings with something like:

    (defun tsang-quick-converter (dicbuf tsang-p big5-p)
      (let* ((charset (if big5-p 'chinese-big5-1 'chinese-cns11643-1))
             (fulltitle
              (propertize (if tsang-p "倉頡" "簡易")
                          'charset charset))

Tho I'm not sure even that would be sufficient, since that function
generates a file so if it just prints those strings into an Elisp file,
the info would again be lost, at least when that Elisp file
gets compiled.

Given that we lived blissfully unaware of the problem for the last 10
years (plus another year with some vague awareness of it but still
without doing anything about it), I suggest we get rid of the `if
big5-p` tests and switch the file to `utf-8`.


        Stefan

Re: Broken `if big5-p` code in titdic-cnv.el

[Andreas Schwab]

On Jan 26 2021, Stefan Monnier wrote:

> the .elc file because when Emacs byte-compiles that code the
> byte-compiler considers those two strings as "equal" and emits only one
> string in the byte-code

Isn't that a bug?

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."

Re: Broken `if big5-p` code in titdic-cnv.el (was: Scan of broken conditional forms)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Kenichi Handa <handa@m17n.org>, Eli Zaretskii <eliz@gnu.org>,
>   mattiase@acm.org,  emacs-devel@gnu.org
> Date: Tue, 26 Jan 2021 22:02:35 -0500
> 
> So, I think using `iso-2022-jp` is a bad idea here: it gives the
> illusion that the two branches are different where they really aren't.
> If we do want to recover the difference (the one we presumably lost in
> Emacs-23), we need to make those two branches return
> properly-propertized strings with something like:
> 
>     (defun tsang-quick-converter (dicbuf tsang-p big5-p)
>       (let* ((charset (if big5-p 'chinese-big5-1 'chinese-cns11643-1))
>              (fulltitle
>               (propertize (if tsang-p "倉頡" "簡易")
>                           'charset charset))
> 
> Tho I'm not sure even that would be sufficient, since that function
> generates a file so if it just prints those strings into an Elisp file,
> the info would again be lost, at least when that Elisp file
> gets compiled.
> 
> Given that we lived blissfully unaware of the problem for the last 10
> years (plus another year with some vague awareness of it but still
> without doing anything about it), I suggest we get rid of the `if
> big5-p` tests and switch the file to `utf-8`.

I've discussed this with Handa-san a year ago, and we arrived at the
conclusion that the charset information is indeed no longer important.

However, if you look carefully at the part of tsang-quick-converter
that begins with

    (let ((punctuation '((";" "；﹔，、﹐﹑" "；﹔，、﹐﹑")

and ends with

    (dolist (elt punctuation)
      (insert (format "(%S %S)\n" (concat "z" (car elt))
		      (if big5-p (nth 1 elt) (nth 2 elt))))))

you will see that some of the characters in the punctuation structure
are actually different between the big5-p and non-big5-p branches,
although most of them are identical.  So either these are artifacts of
converting this file from its original encoding, or there are actual
differences between these two branches, and we cannot simply delete
one of them.

This puzzle has been sitting in my TODO since I discovered these
differences a year ago.  If you (or someone else) are willing to
unlock the mystery and simplify the file accordingly, that would be
welcome indeed.

Re: Broken `if big5-p` code in titdic-cnv.el

[Stefan Monnier]

> I've discussed this with Handa-san a year ago, and we arrived at the
> conclusion that the charset information is indeed no longer important.

Thanks.

> However, if you look carefully at the part of tsang-quick-converter
> that begins with
>
>     (let ((punctuation '((";" "；﹔，、﹐﹑" "；﹔，、﹐﹑")
>
> and ends with
>
>     (dolist (elt punctuation)
>       (insert (format "(%S %S)\n" (concat "z" (car elt))
> 		      (if big5-p (nth 1 elt) (nth 2 elt))))))
>
> you will see that some of the characters in the punctuation structure
> are actually different between the big5-p and non-big5-p branches,
> although most of them are identical.

That's indeed what I noticed a few minutes ago.  I don't think it
affects the decision of what to do in the short term: I pushed the patch
below (additionally to converting the file back to utf-8, which doesn't
render nicely in a patch).

> So either these are artifacts of converting this file from its
> original encoding, or there are actual differences between these two
> branches, and we cannot simply delete one of them.

AFAIK this `punctuation` just adds some additional entries to the input
method beside the entries extracted from the source files in
leim/MISC-DIC.  I think it's OK to add "almost exactly the same but not
quite" to the CNS and BIG5 input methods since they are themselves
pretty close to each other already (yet different), AFAIU.

According to `ediff-buffers`, the sources `cangjie-table.b5` and
`cangjie-table.cns` have only 6 characters different, so maybe it's not
worth keeping them separate and we should actually merge those input
methods:

    chinese-b5-quick    and chinese-cns-quick      =>  chinese-quick
    chinese-b5-tsangchi and chinese-cns-tsangchi   =>  chinese-tsangchi

In any case, I'm not in a position to make such a decision since I don't
know anything about those scripts and even less about how they're used.
So I only installed changes which shouldn't affect the actual
behavior of those input methods.


        Stefan


diff --git a/lisp/international/titdic-cnv.el b/lisp/international/titdic-cnv.el
index 84e218f179..ce5c04293a 100644
--- a/lisp/international/titdic-cnv.el
+++ b/lisp/international/titdic-cnv.el
@@ -375,7 +375,7 @@ tit-process-header
     ;; Arg DOCSTRING
     (let ((doc (concat tit-prompt "\n"))
 	  (comments (if tit-comments
-			(mapconcat 'identity (nreverse tit-comments) "\n")))
+			(mapconcat #'identity (nreverse tit-comments) "\n")))
 	  (doc-ext (nth 2 (assoc package quail-cxterm-package-ext-info))))
       (if comments
 	  (setq doc (concat doc "\n" comments "\n")))
@@ -737,12 +737,10 @@ quail-misc-package-ext-info
 ;; method is for inputting CNS characters.
 
 (defun tsang-quick-converter (dicbuf tsang-p big5-p)
-  (let ((fulltitle (if tsang-p (if big5-p "倉頡" "倉頡")
-		     (if big5-p "簡易" "簡易")))
+  (let ((fulltitle (if tsang-p "倉頡" "簡易"))
 	dic)
     (goto-char (point-max))
-    (if big5-p
-	(insert (format "\"中文輸入【%s】BIG5
+    (insert (format "\"中文輸入【%s】%s
 
 	漢語%s輸入鍵盤
 
@@ -753,19 +751,7 @@ tsang-quick-converter
       [Z  ] [X 難] [C 金] [V 女] [B 月] [N 弓] [M 一]
 
 \\\\<quail-translation-docstring>\"\n"
-			fulltitle fulltitle))
-      (insert (format "\"中文輸入【%s】CNS
-
-	漢語%s輸入鍵盤
-
-   [Q 手] [W 田] [E 水] [R 口] [T 廿] [Y 卜] [U 山] [I 戈] [O 人] [P 心]
-
-    [A 日] [S 尸] [D 木] [F 火] [G 土] [H 竹] [J 十] [L 中]
-
-      [Z  ] [X 難] [C 金] [V 女] [B 月] [N 弓] [M 一]
-
-\\\\<quail-translation-docstring>\"\n"
-		      fulltitle fulltitle)))
+		    fulltitle (if big5-p "BIG5" "CNS") fulltitle))
     (insert "  '((\".\" . quail-next-translation-block)
    (\",\" . quail-prev-translation-block))
   nil nil)\n\n")
@@ -953,7 +939,7 @@ ziranma-converter
 				     (= (length (aref trans i)) 1))
 			   (setq i (1+ i)))
 			 (if (= i len)
-			     (setq trans (mapconcat 'identity trans "")))))
+			     (setq trans (mapconcat #'identity trans "")))))
 		     (setq dic (cons (cons key trans) dic)))
 		 table)))
     (setq dic (sort dic (lambda (x y) (string< (car x) (car y)))))