Fixing numerous `message' bugs..

Fixing numerous `message' bugs..

[D. Goel]

The emacs source code is littered with thousands of buggy calls to
`message'.  Here is an example of such a structure:


(message 
	(if foo
		nil
		(if bar "abc"
			(concat "def" filename))))

This example will lead to an error if the filename has %s in it. 

At the same time, the simplistic fix of replacing (message by (message
"%s" would be incorrect because if foo is true, the coder wanted
(message nil), and not (message "%s" nil).


The appropriate fix to this would be

(let ((arg ((rest-of-the-code))))
	(if (null arg)
	(message nil)
	(message "%s" arg)))

^^^ This fix will have to be repeated thousands of time as I go
through the source code.

It would rather make much more sense to code this fix into a little
function:

(defun msg (arg)
	(if (null arg)
	(message nil)
	(message (format "%s" arg))))


Then, I simply have to replace buggy calls to `message' with `msg'

----

.. this new function can be further improved to be more general, so
that develepors can simply start preferring the new msg if they like -

(defun msg (&rest args)
     (cond
      ((null args) (message nil))
      ((null (car args) (message nil)))
      ((= (length args) 1) (message "%s" (car args)))
      (apply 'message args)))


I would like to add `msg' to simple.el, and replace buggy `message'
calls by `msg'.  Can you please confirm or comment?


((My correct email is now deego3@gmail.com, please cc there as well;
emacs-devel seems to send those messages to spam, so I am emailing
from my older gnufans address.  If an admin sees this, can you please
unblock my other email to emacs-devel sent earlier today about a
bugfix I made to find-func.el? thanks.)

- deego (Deepak Goel)

Re: Fixing numerous `message' bugs..

[David Kastrup]

D.  Goel <deego@gnufans.org> writes:

> The emacs source code is littered with thousands of buggy calls to
> `message'.  Here is an example of such a structure:
>
>
> (message 
> 	(if foo
> 		nil
> 		(if bar "abc"
> 			(concat "def" filename))))
>
> This example will lead to an error if the filename has %s in it. 
>
> At the same time, the simplistic fix of replacing (message by (message
> "%s" would be incorrect because if foo is true, the coder wanted
> (message nil), and not (message "%s" nil).
>
>
> The appropriate fix to this would be
>
> (let ((arg ((rest-of-the-code))))
> 	(if (null arg)
> 	(message nil)
> 	(message "%s" arg)))

(message (unless foo (if bar "abc" "def%s")) filename)

Note that it is perfectly fine to have spurious trailing arguments to
message.

>
> ^^^ This fix will have to be repeated thousands of time as I go
> through the source code.
>
> It would rather make much more sense to code this fix into a little
> function:
>
> (defun msg (arg)
> 	(if (null arg)
> 	(message nil)
> 	(message (format "%s" arg))))
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^

Uh, no.  You mean (message "%s" arg) here, and anyway:

(defun msg (arg)
  (message (and arg "%s") arg))

and then it is simple enough that it is not worth bothering introducing
a separate function.  Of course, arg should not involve a complex
calculation.

> .. this new function can be further improved to be more general, so
> that develepors can simply start preferring the new msg if they like -
>
> (defun msg (&rest args)
>      (cond
>       ((null args) (message nil))
>       ((null (car args) (message nil)))
>       ((= (length args) 1) (message "%s" (car args)))
>       (apply 'message args)))

I think that is nonsensical.  Problems occur only when a message
contains non-fixed material (which may or may not contain percentages),
but a message will pretty much never contain _exclusively_ non-fixed
material.  So a good solution will almost always involve more than a
trivial "%s" format.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Fixing numerous `message' bugs..

[Dave Goel]

David Kastrup <dak@gnu.org> writes:

> D.  Goel <deego@gnufans.org> writes:
>>
>> (let ((arg ((rest-of-the-code))))
>> 	(if (null arg)
>> 	(message nil)
>> 	(message "%s" arg)))
>
> (message (unless foo (if bar "abc" "def%s")) filename)

This alternative fix relies on going deep inside developer's
"rest-of-the-code".  This is a wrong solution.  What will you do if
that rest-of-the-code relies on strings constructed in other
functions?  (That is just a simple example. Emacs is full of very
creative codes, constructed in macros and many other beautiful things,
and strings read from assoc lists...) Change the code in those other
functions?  What if the other functions are used elsewhere? To be
safe, I should perhaps duplicate the 10000-char assoc-list into an
assoc-list-called-by-message and make your fix there?  Rather than
duplicate many such solutions and add thousands of lines of code,
isn't it easier to add one small function `msg'?  Even otherwise, the
(let ((arg "rest-of-the-code"..)))  solution itself would add a lot of
extra verbiage, much more so than a simple function `msg'.

This is also an impractical solution, 
(a) My semi-automated code-fixers cannot handle this.
(b) This involves a lot of studying of code.

I mentioned these bugs in calls to `message' and `error' were
identified probably more than a year ago; I have fixed a lot of these
bugs, but something like 90% of these bugs remain unfixed... the
problem was so bad that edebug itself had these bugs, so that
edebugging itself broke mysteriously at times.  If I can introduce
(msg), I can fix all these bugs right now.



> Note that it is perfectly fine to have spurious trailing arguments to
> message.

sure.

>
> (defun msg (arg)
>   (message (and arg "%s") arg))

nice.

> and then it is simple enough that it is not worth bothering introducing
> a separate function.  

No, it isn't.  You identify the problem below yourself.

> Of course, arg should not involve a complex calculation.

And, of course, they do.  They also involve calls to other functions,
etc.

>
>> .. this new function can be further improved to be more general, so
>> that develepors can simply start preferring the new msg if they like -
>>
>> (defun msg (&rest args)
>>      (cond
>>       ((null args) (message nil))
>>       ((null (car args)) (message nil))
>>       ((= (length args) 1) (message "%s" (car args)))
>>       (apply 'message args)))
>
> I think that is nonsensical.  Problems occur only when a message
> contains non-fixed material

Yeah, which is the case a lot of those times. And, that is the
problem. 

> (which may or may not contain percentages), but a message will
> pretty much never contain _exclusively_ non-fixed material.  

huh? 

> So a good solution will almost always involve more than a trivial
> "%s" format.

If you really want something more than %s, say, %% [1], no one stops you
from still using message correctly, as follows:

(message "%%")  

or even
(msg (format "%%"))

Most of the time a developer calls (message), he calls it with one
arg, and he already pre-formats his arg.  Almost always when the
developer calls (message) with one arg, he is actually looking for
(msg)[2].   And, if he isn't, he is still welcome to use (message) the
correct way. 


[1] though I assure you, each of the thousand times I have seen a
(message) bug, the developer has always meant that "trivial" %s.

[2] Once again, in the thousands of cases I have examined, nowhere
have I seen (message) called with just one non-nil argument, where
(message "%s" arg) fails to do what the developer wanted.

---

Please note that I am *not* saying I want to replace every call to
(message) in the source code with (msg).  *Only* the buggy calls; and
only when I have verified carefully that (msg) works correctly for the
code at hand.

Re: Fixing numerous `message' bugs..

[Stefan Monnier]

> It would rather make much more sense to code this fix into a little
> function:

> (defun msg (arg)
> 	(if (null arg)
> 	(message nil)
> 	(message (format "%s" arg))))

Another solution is to change `message' so that if it has only one
argument it behaves as if it had received a "%s" format string.

This will automatically fix all such code.  The only problem with it is
that there is code out there that calls message with a single arg
and expects `message' to dequote "%%" into "%".


        Stefan

Re: Fixing numerous `message' bugs..

[Dave Goel]

>
> Another solution is to change `message' so that if it has only one
> argument it behaves as if it had received a "%s" format string.

(provided that that argument is non-nil)


> This will automatically fix all such code.  The only problem with it is
> that there is 

.. (in principle)..

> code out there that calls message with a single arg and expects
> `message' to dequote "%%" into "%".


Indeed.   We should leave `message' itelf alone.

Re: Fixing numerous `message' bugs..

[Stefan Monnier]

>> This will automatically fix all such code.  The only problem with it is
>> that there is 

> .. (in principle)..

Not just in principle.  A quick "grep '(message.*%%' lisp/**/*.el" shows
a fair number.

>> code out there that calls message with a single arg and expects
>> `message' to dequote "%%" into "%".

> Indeed.   We should leave `message' itelf alone.

Actually, I'd prefer changing message, but I already suggested it and it
was rejected (maybe I even installed the change and then backed it out,
can't remember).
The problem I wanted to address with this change was not just to fix
existing code but to try and avoid such bugs in the future as well.


        Stefan

Re: Fixing numerous `message' bugs..

[David Kastrup]

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> This will automatically fix all such code.  The only problem with it is
>>> that there is 
>
>> .. (in principle)..
>
> Not just in principle.  A quick "grep '(message.*%%' lisp/**/*.el" shows
> a fair number.
>
>>> code out there that calls message with a single arg and expects
>>> `message' to dequote "%%" into "%".
>
>> Indeed.   We should leave `message' itelf alone.
>
> Actually, I'd prefer changing message, but I already suggested it and it
> was rejected (maybe I even installed the change and then backed it out,
> can't remember).
> The problem I wanted to address with this change was not just to fix
> existing code but to try and avoid such bugs in the future as well.

How about the following (hopefully more or less exhaustive) patch?  Not
all occurences might have the proper caution (or too much) regarding
possible nil arguments, though.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: xxxx --]
[-- Type: text/x-patch, Size: 26856 bytes --]

diff --git a/lisp/calc/calc-ext.el b/lisp/calc/calc-ext.el
index 140335a..56b95ef 100644
--- a/lisp/calc/calc-ext.el
+++ b/lisp/calc/calc-ext.el
@@ -1434,7 +1434,7 @@ calc-kill calc-kill-region calc-yank))))
      (calc-set-command-flag 'no-align)
      (setq prefix (set flag (not (symbol-value flag)))
 	   prefix-arg n)
-     (message (if prefix msg "")))
+     (message "%s" (if prefix msg "")))
     (and prefix
 	 (not calc-is-keypad-press)
 	 (if (boundp 'overriding-terminal-local-map)
diff --git a/lisp/calc/calc-mode.el b/lisp/calc/calc-mode.el
index d7daf1b..85ae4d7 100644
--- a/lisp/calc/calc-mode.el
+++ b/lisp/calc/calc-mode.el
@@ -505,7 +505,7 @@
 			     mode)
 		      (and (not (eq calc-simplify-mode mode))
 			   mode)))
-  (message (if (eq calc-simplify-mode mode)
+  (message "%s" (if (eq calc-simplify-mode mode)
 	       msg
 	     "Default simplifications enabled")))
 
diff --git a/lisp/calc/calc.el b/lisp/calc/calc.el
index 69cacec..6e12fbe 100644
--- a/lisp/calc/calc.el
+++ b/lisp/calc/calc.el
@@ -1228,7 +1228,7 @@ If nil, selections displayed but ignored.")
   (let ((prompt2 (format "%s " (key-description (this-command-keys))))
 	(glob (current-global-map))
 	(loc (current-local-map)))
-    (or (input-pending-p) (message prompt))
+    (or (input-pending-p) (message "%s" prompt))
     (let ((key (calc-read-key t)))
       (calc-unread-command (cdr key))
       (unwind-protect
@@ -1244,7 +1244,7 @@ If nil, selections displayed but ignored.")
 (defun calc-version ()
   "Return version of this version of Calc."
   (interactive)
-  (message (concat "Calc version " calc-version)))
+  (message "Calc version %s" calc-version))
 
 (defun calc-mode ()
   "Calculator major mode.
diff --git a/lisp/calc/calcalg2.el b/lisp/calc/calcalg2.el
index 4afed43..1259642 100644
--- a/lisp/calc/calcalg2.el
+++ b/lisp/calc/calcalg2.el
@@ -746,7 +746,7 @@
 		      (setq math-integ-msg (format
 					    "Working... Integrating %s"
 					    (math-format-flat-expr expr 0)))
-		      (message math-integ-msg)))
+		      (message "%s" math-integ-msg)))
 		(if math-cur-record
 		    (setcar (cdr math-cur-record)
 			    (if same-as-above (vector simp) 'busy))
@@ -773,7 +773,7 @@
 						     "simplification...\n")
 			      (setq val (math-integral simp 'no t))))))))
 	      (if (eq calc-display-working-message 'lots)
-		  (message math-integ-msg)))
+		  (message "%s" math-integ-msg)))
 	  (setcar (cdr math-cur-record) (or val
 				       (if (or math-enable-subst
 					       (not math-any-substs))
diff --git a/lisp/calc/calcalg3.el b/lisp/calc/calcalg3.el
index 374b048..77e8b15 100644
--- a/lisp/calc/calcalg3.el
+++ b/lisp/calc/calcalg3.el
@@ -416,7 +416,7 @@
              (calc-record (calc-normalize calc-fit-to-trail) "parm"))))
   (when plot
     (if (stringp plot)
-        (message plot)
+        (message "%s" plot)
       (let ((calc-graph-no-auto-view t))
         (calc-graph-delete t)
         (calc-graph-add-curve
diff --git a/lisp/calendar/calendar.el b/lisp/calendar/calendar.el
index d49667d..da87a19 100644
--- a/lisp/calendar/calendar.el
+++ b/lisp/calendar/calendar.el
@@ -3154,7 +3154,7 @@ Defaults to today's date if DATE is not given."
 (defun calendar-print-day-of-year ()
   "Show day number in year/days remaining in year for date under the cursor."
   (interactive)
-  (message (calendar-day-of-year-string (calendar-cursor-to-date t))))
+  (message "%s" (calendar-day-of-year-string (calendar-cursor-to-date t))))
 
 (defun calendar-set-mode-line (str)
   "Set mode line to STR, centered, surrounded by dashes."
diff --git a/lisp/emacs-lisp/checkdoc.el b/lisp/emacs-lisp/checkdoc.el
index 185df6f..742e134 100644
--- a/lisp/emacs-lisp/checkdoc.el
+++ b/lisp/emacs-lisp/checkdoc.el
@@ -978,7 +978,7 @@ Optional argument INTERACT permits more interactive fixing."
     (if (not (interactive-p))
 	e
       (if e
-	  (message (checkdoc-error-text e))
+	  (message "%s" (checkdoc-error-text e))
 	(checkdoc-show-diagnostics)
 	(message "Space Check: done.")))))
 
@@ -1038,15 +1038,15 @@ space at the end of each line."
 	     (end (save-excursion (end-of-defun) (point)))
 	     (msg (checkdoc-this-string-valid)))
 	(if msg (if no-error
-		    (message (checkdoc-error-text msg))
+		    (message "%s" (checkdoc-error-text msg))
 		  (error "%s" (checkdoc-error-text msg)))
 	  (setq msg (checkdoc-message-text-search beg end))
 	  (if msg (if no-error
-		      (message (checkdoc-error-text msg))
+		      (message "%s" (checkdoc-error-text msg))
 		    (error "%s" (checkdoc-error-text msg)))
 	    (setq msg (checkdoc-rogue-space-check-engine beg end))
 	    (if msg (if no-error
-			(message (checkdoc-error-text msg))
+			(message "%s" (checkdoc-error-text msg))
 		      (error "%s" (checkdoc-error-text msg))))))
 	(if (interactive-p) (message "Checkdoc: done."))))))
 
diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index e3ade01..4dc7bdb 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -3295,12 +3295,12 @@ With prefix argument, make it a temporary breakpoint."
   (if (eq (1+ edebug-recursion-depth) (recursion-depth))
       (progn
 	(setq edebug-execution-mode mode)
-	(message shortmsg)
+	(message "%s" shortmsg)
 	;; Continue execution
 	(exit-recursive-edit))
     ;; This is not terribly useful!!
     (setq edebug-next-execution-mode mode)
-    (message msg)))
+    (message "%s" msg)))
 
 
 (defalias 'edebug-step-through-mode 'edebug-step-mode)
diff --git a/lisp/emacs-lisp/lisp-mnt.el b/lisp/emacs-lisp/lisp-mnt.el
index ea6a96c..c9dc657 100644
--- a/lisp/emacs-lisp/lisp-mnt.el
+++ b/lisp/emacs-lisp/lisp-mnt.el
@@ -562,7 +562,7 @@ copyright notice is allowed."
 	       (t
 		ret)))))
     (if verbose
-	(message ret))
+	(message (and ret "%s") ret))
     ret))
 
 (defun lm-synopsis (&optional file showall)
diff --git a/lisp/emulation/viper-util.el b/lisp/emulation/viper-util.el
index 87bf523..f496d34 100644
--- a/lisp/emulation/viper-util.el
+++ b/lisp/emulation/viper-util.el
@@ -636,7 +636,7 @@
 	 (regexp (format "^[^;]*%s[ \t\n]*[a-zA-Z---_']*[ \t\n)]" var-name))
 	 (buf (find-file-noselect (substitute-in-file-name custom-file)))
 	)
-    (message message)
+    (message (and message "%s") message)
     (save-excursion
       (set-buffer buf)
       (goto-char (point-min))
diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el
index c0f4205..04ddab1 100644
--- a/lisp/erc/erc-backend.el
+++ b/lisp/erc/erc-backend.el
@@ -515,7 +515,7 @@ We will store server variables in the buffer given by BUFFER."
       (set-process-filter process 'erc-server-filter-function)
       (set-process-buffer process buffer)))
   (erc-log "\n\n\n********************************************\n")
-  (message (erc-format-message
+  (message "%s" (erc-format-message
             'login ?n
             (with-current-buffer buffer (erc-current-nick))))
   ;; wait with script loading until we receive a confirmation (first
diff --git a/lisp/erc/erc-lang.el b/lisp/erc/erc-lang.el
index e61156f..b11721d 100644
--- a/lisp/erc/erc-lang.el
+++ b/lisp/erc/erc-lang.el
@@ -197,7 +197,7 @@ Normungsinstitut (ON), Postfach 130, A-1021 Vienna, Austria.")
   "Return the language name for the ISO CODE."
   (interactive (list (completing-read "ISO language code: "
 				      iso-638-languages)))
-  (message (cdr (assoc code iso-638-languages))))
+  (message "%s" (cdr (assoc code iso-638-languages))))
 
 (defun erc-cmd-LANG (language)
   "Display the language name for the language code given by LANGUAGE."
diff --git a/lisp/erc/erc-stamp.el b/lisp/erc/erc-stamp.el
index 64b0405..ae9bb51 100644
--- a/lisp/erc/erc-stamp.el
+++ b/lisp/erc/erc-stamp.el
@@ -412,8 +412,8 @@ NOW is position of point currently."
   (when erc-echo-timestamps
     (let ((stamp (get-text-property now 'timestamp)))
       (when stamp
-	(message (format-time-string erc-echo-timestamp-format
-				     stamp))))))
+	(message "%s" (format-time-string erc-echo-timestamp-format
+					  stamp))))))
 
 (provide 'erc-stamp)
 
diff --git a/lisp/gnus/ecomplete.el b/lisp/gnus/ecomplete.el
index 42a7591..285aca4 100644
--- a/lisp/gnus/ecomplete.el
+++ b/lisp/gnus/ecomplete.el
@@ -119,7 +119,7 @@
 	  nil)
       (if (not choose)
 	  (progn
-	    (message matches)
+	    (message "%s" matches)
 	    nil)
 	(setq highlight (ecomplete-highlight-match-line matches line))
 	(while (not (memq (setq command (read-event highlight)) '(? return)))
diff --git a/lisp/gnus/gnus-sum.el b/lisp/gnus/gnus-sum.el
index b082a8b..826fdd7 100644
--- a/lisp/gnus/gnus-sum.el
+++ b/lisp/gnus/gnus-sum.el
@@ -4028,7 +4028,7 @@ If NO-DISPLAY, don't generate a summary buffer."
   "Query where the respool algorithm would put this article."
   (interactive)
   (gnus-summary-select-article)
-  (message (gnus-general-simplify-subject (gnus-summary-article-subject))))
+  (message "%s" (gnus-general-simplify-subject (gnus-summary-article-subject))))
 
 (defun gnus-gather-threads-by-subject (threads)
   "Gather threads by looking at Subject headers."
diff --git a/lisp/international/ja-dic-cnv.el b/lisp/international/ja-dic-cnv.el
index e7c9b63..d7add6f 100644
--- a/lisp/international/ja-dic-cnv.el
+++ b/lisp/international/ja-dic-cnv.el
@@ -558,7 +558,7 @@ To get complete usage, invoke:
 	 (while l
 	   (setq count (1+ count))
 	   (if (= (% count 10000) 0)
-	       (message (format "%d entries" count)))
+	       (message "%d entries" count))
 	   (setq entry (skkdic-extract-conversion-data (car l)))
 	   (set-nested-alist (car entry) (cdr entry) map)
 	   (setq l (cdr l)))
diff --git a/lisp/kmacro.el b/lisp/kmacro.el
index 096d35b..ebb3a4a 100644
--- a/lisp/kmacro.el
+++ b/lisp/kmacro.el
@@ -404,7 +404,7 @@ Optional arg EMPTY is message to print if no macros are defined."
 		   (format " [%s]"
 			   (format kmacro-counter-format-start kmacro-counter)))
 		 (if z (substring m 0 (1- x)) m) (if z "..." "")))
-    (message (or empty "No keyboard macros defined"))))
+    (message "%s" (or empty "No keyboard macros defined"))))
 
 
 (defun kmacro-repeat-on-last-key (keys)
diff --git a/lisp/mail/reporter.el b/lisp/mail/reporter.el
index 24dd9ab..0777c60 100644
--- a/lisp/mail/reporter.el
+++ b/lisp/mail/reporter.el
@@ -118,7 +118,7 @@ composed.")
   "Periodically output a status message."
   (if (zerop (% reporter-status-count 10))
       (progn
-	(message reporter-status-message)
+	(message "%s" reporter-status-message)
 	(setq reporter-status-message (concat reporter-status-message "."))))
   (setq reporter-status-count (1+ reporter-status-count)))
 
diff --git a/lisp/net/rcirc.el b/lisp/net/rcirc.el
index ef24de4..a1a0e0c 100644
--- a/lisp/net/rcirc.el
+++ b/lisp/net/rcirc.el
@@ -1708,13 +1708,13 @@ With prefix ARG, go to the next low priority buffer with activity."
 	    (recenter -1)))
       (if (eq major-mode 'rcirc-mode)
 	  (switch-to-buffer (rcirc-non-irc-buffer))
-	(message (concat
-		  "No IRC activity."
-		  (when lopri
-		    (concat
-		     "  Type C-u "
-		     (key-description (this-command-keys))
-		     " for low priority activity."))))))))
+	(message "%s" (concat
+		       "No IRC activity."
+		       (when lopri
+			 (concat
+			  "  Type C-u "
+			  (key-description (this-command-keys))
+			  " for low priority activity."))))))))
 
 (defvar rcirc-activity-hooks nil
   "Hook to be run when there is channel activity.
diff --git a/lisp/play/mpuz.el b/lisp/play/mpuz.el
index 4cd3bd6..8b40b41 100644
--- a/lisp/play/mpuz.el
+++ b/lisp/play/mpuz.el
@@ -489,7 +489,7 @@ You may abort a game by typing \\<mpuz-mode-map>\\[mpuz-offer-abort]."
 			       ((< mpuz-nb-errors 10)	"bad!")
 			       ((< mpuz-nb-errors 15)	"awful.")
 			       (t			"not serious.")))))
-    (message message)
+    (message "%s" message)
     (sit-for 4)
     (if (y-or-n-p (concat message "  Start a new game? "))
 	(mpuz-start-new-game)
diff --git a/lisp/progmodes/antlr-mode.el b/lisp/progmodes/antlr-mode.el
index 9f6e70d..3750eed 100644
--- a/lisp/progmodes/antlr-mode.el
+++ b/lisp/progmodes/antlr-mode.el
@@ -1875,7 +1875,7 @@ cell where the two values determine the area inside the braces."
 					(read initial)
 				      initial))
 			       (cdr value))))
-	(message (cadr value))
+	(message (and (cadr value) "%s") (cadr value))
 	(setq value nil)))
     ;; insert value ----------------------------------------------------------
     (if (consp old)
diff --git a/lisp/progmodes/cperl-mode.el b/lisp/progmodes/cperl-mode.el
index 4a397a9..4f10f4f 100644
--- a/lisp/progmodes/cperl-mode.el
+++ b/lisp/progmodes/cperl-mode.el
@@ -4581,7 +4581,7 @@ the sections using `cperl-pod-head-face', `cperl-pod-face',
 					   (setq qtag "Can't find })")))
 				  (progn
 				    (goto-char (1- e))
-				    (message qtag))
+				    (message "%s" qtag))
 				(cperl-postpone-fontification
 				 (1- tag) (1- (point))
 				 'face font-lock-variable-name-face)
diff --git a/lisp/progmodes/ebrowse.el b/lisp/progmodes/ebrowse.el
index 5acbe70..3fe83d9 100644
--- a/lisp/progmodes/ebrowse.el
+++ b/lisp/progmodes/ebrowse.el
@@ -896,10 +896,10 @@ this is the first progress message displayed."
   (let (message-log-max)
     (when start (setq ebrowse-n-boxes 0))
     (setq ebrowse-n-boxes (mod (1+ ebrowse-n-boxes) ebrowse-max-boxes))
-    (message (concat title ": "
-		     (propertize (make-string ebrowse-n-boxes
-					      (if (display-color-p) ?\  ?+))
-				 'face 'ebrowse-progress)))))
+    (message "%s: %s" title
+	     (propertize (make-string ebrowse-n-boxes
+				      (if (display-color-p) ?\  ?+))
+			 'face 'ebrowse-progress))))
 
 \f
 ;;; Reading a tree from disk
diff --git a/lisp/progmodes/idlwave.el b/lisp/progmodes/idlwave.el
index 9648494..49e8859 100644
--- a/lisp/progmodes/idlwave.el
+++ b/lisp/progmodes/idlwave.el
@@ -3799,7 +3799,7 @@ unless the optional second argument NOINDENT is non-nil."
       (if (not noindent)
 	  (indent-region beg end nil))
       (if (stringp prompt)
-	  (message prompt)))))
+	  (message "%s" prompt)))))
 
 (defun idlwave-rw-case (string)
   "Make STRING have the case required by `idlwave-reserved-word-upcase'."
@@ -7038,7 +7038,7 @@ sort the list before displaying"
 			 (select-window win)
 			 (eval idlwave-complete-after-success-form))
 		     (set-window-start cwin (point-min)))))
-	  (and message (message message)))
+	  (and message (message "%s" message)))
       (select-window win))))
 
 (defun idlwave-display-completion-list (list &optional message beg complete)
@@ -7069,7 +7069,7 @@ sort the list before displaying"
   (run-hooks 'idlwave-completion-setup-hook)
 
   ;; Display the message
-  (message (or message "Making completion list...done")))
+  (message "%s" (or message "Making completion list...done")))
 
 (defun idlwave-choose (function &rest args)
   "Call FUNCTION as a completion chooser and pass ARGS to it."
diff --git a/lisp/progmodes/vhdl-mode.el b/lisp/progmodes/vhdl-mode.el
index df315e4..a9f93b4 100644
--- a/lisp/progmodes/vhdl-mode.el
+++ b/lisp/progmodes/vhdl-mode.el
@@ -10432,7 +10432,7 @@ with double-quotes is to be inserted.  DEFAULT specifies a default string."
   "Query a decision from the user."
   (let ((start (point)))
     (when string (vhdl-insert-keyword (concat string " ")))
-    (message prompt)
+    (message (and prompt "%s") prompt)
     (let ((char (read-char)))
       (delete-region start (point))
       (if (and optional (eq char ?\r))
diff --git a/lisp/replace.el b/lisp/replace.el
index 34fdd5f..d290275 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -1405,19 +1405,29 @@ with the `noescape' argument set.
   "Like `match-data', but markers in REUSE get invalidated.
 If NEW is non-nil, it is set and returned instead of fresh data,
 but coerced to the correct value of INTEGERS."
-  (or (and new
+  (if (and new
 	   (progn
 	     (set-match-data new)
-	     (and (eq new reuse)
-		  (eq (null integers) (markerp (car reuse)))
-		  new)))
-      (match-data integers reuse t)))
-
-(defun replace-match-maybe-edit (newtext fixedcase literal noedit match-data)
+	     (eq new reuse))
+	   (cond
+	    ((markerp (car new)) (null integers))
+	    ((bufferp (car (last new))) integers)
+	    (t)))
+      new
+    (match-data integers reuse t)))
+
+(defun replace-match-maybe-edit (newtext fixedcase literal noedit match-data
+					 &optional string)
   "Make a replacement with `replace-match', editing `\\?'.
-NEWTEXT, FIXEDCASE, LITERAL are just passed on.  If NOEDIT is true, no
-check for `\\?' is made to save time.  MATCH-DATA is used for the
-replacement.  In case editing is done, it is changed to use markers.
+NEWTEXT, FIXEDCASE, LITERAL are just passed on.  If NOEDIT is
+true, no check for `\\?' is made to save time.  MATCH-DATA is
+used for the replacement.  Optional STRING indicates a source
+string to perform the modification in, if nil, the current bugger
+is used.
+
+In case replacement string editing is offered, buffer-based
+match-data is changed to use markers since one can temporarily
+visit the buffer for edits while editing the replacement string.
 
 The return value is non-nil if there has been no `\\?' or NOEDIT was
 passed in.  If LITERAL is set, no checking is done, anyway."
@@ -1436,7 +1446,7 @@ passed in.  If LITERAL is set, no checking is done, anyway."
                                   nil match-data match-data))))
 	    noedit nil)))
   (set-match-data match-data)
-  (replace-match newtext fixedcase literal)
+  (replace-match newtext fixedcase literal string)
   noedit)
 
 (defun perform-replace (from-string replacements
diff --git a/lisp/startup.el b/lisp/startup.el
index 366491f..0f0bfd9 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -1537,7 +1537,7 @@ splash screen in another window."
 	  (apply #'fancy-splash-insert text)
 	  (insert "\n"))
 	(unless (current-message)
-	  (message fancy-splash-help-echo))
+	  (message (and fancy-splash-help-echo "%s") fancy-splash-help-echo))
 	(set-buffer-modified-p nil)
 	(goto-char (point-min))
 	(force-mode-line-update))
diff --git a/lisp/term/mac-win.el b/lisp/term/mac-win.el
index 1f00939..b6db7a2 100644
--- a/lisp/term/mac-win.el
+++ b/lisp/term/mac-win.el
@@ -2178,7 +2178,7 @@ either in the current buffer or in the echo area."
     (if (not buffer-read-only)
 	(insert text)
       (kill-new text)
-      (message
+      (message "%s"
        (substitute-command-keys
 	"The text from the Services menu can be accessed with \\[yank]")))))
 
diff --git a/lisp/textmodes/artist.el b/lisp/textmodes/artist.el
index f044290..12c4d6f 100644
--- a/lisp/textmodes/artist.el
+++ b/lisp/textmodes/artist.el
@@ -1151,14 +1151,14 @@ PREV-OP-ARG are used when invoked recursively during the build-up."
   (interactive)
   (let ((next-op (cdr (cdr (assoc artist-curr-go artist-prev-next-op-alist)))))
     (artist-select-operation next-op)
-    (message next-op)))
+    (message (and next-op "%s") next-op)))
 
 (defun artist-select-prev-op-in-list ()
   "Cyclically select previous drawing mode operation."
   (interactive)
   (let ((prev-op (car (cdr (assoc artist-curr-go artist-prev-next-op-alist)))))
     (artist-select-operation prev-op)
-    (message prev-op)))
+    (message (and prev-op "%s") prev-op)))
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
diff --git a/lisp/textmodes/org.el b/lisp/textmodes/org.el
index 0535f67..124f6c4 100644
--- a/lisp/textmodes/org.el
+++ b/lisp/textmodes/org.el
@@ -7487,7 +7487,7 @@ When TAG is non-nil, don't move trees, but mark them with the ARCHIVE tag."
 	(org-hide-archived-subtrees beg end)
 	(goto-char beg)
 	(if (looking-at (concat ".*:" org-archive-tag ":"))
-	    (message (substitute-command-keys
+	    (message "%s" (substitute-command-keys
 		      "Subtree is archived and stays closed.  Use \\[org-force-cycle-archived] to cycle it anyway.")))))))
 
 (defun org-force-cycle-archived ()
@@ -9366,7 +9366,9 @@ of the new mark."
 	  (goto-line l1)))
     (if (not (= epos (point-at-eol))) (org-table-align))
     (goto-line l)
-    (and (interactive-p) (message (cdr (assoc new org-recalc-marks))))))
+    (when (interactive-p)
+      (setq new (cdr (assoc new org-recalc-marks)))
+      (message (and new "%s") new))))
 
 (defun org-table-maybe-recalculate-line ()
   "Recompute the current line if marked for it, and if we haven't just done it."
@@ -12347,7 +12349,7 @@ to read."
   (move-marker (car org-mark-ring)
 	       (or pos (point))
 	       (or buffer (current-buffer)))
-  (message
+  (message "%s"
    (substitute-command-keys
     "Position saved to mark ring, go back with \\[org-mark-ring-goto].")))
 
@@ -12429,7 +12431,7 @@ onto the ring."
         (if (match-string 2 name) ; If there isn't a node, choose "Top"
             (Info-find-node (match-string 1 name) (match-string 2 name))
           (Info-find-node (match-string 1 name) "Top")))
-    (message (concat "Could not open: " name))))
+    (message "Could not open: %s" name)))
 
 (defun org-follow-gnus-link (&optional group article)
   "Follow a Gnus link to GROUP and ARTICLE."
@@ -12553,7 +12555,7 @@ sequences, it will now work."
   (save-excursion
     (mh-index-previous-folder)
     (re-search-forward "^\\(+.*\\)$" nil t)
-    (message (match-string 1))))
+    (message "%s" (match-string 1))))
 
 (defun org-mhe-get-message-folder ()
   "Return the name of the current message folder.  Be careful if you
@@ -13759,7 +13761,7 @@ This function should be run in the `org-after-todo-state-change-hook'."
 	  (org-timestamp-change n (cdr (assoc what whata))))
 	(setq msg (concat msg type org-last-changed-timestamp " ")))
       (setq org-log-post-message msg)
-      (message msg))))
+      (message "%s" msg))))
 
 (defun org-show-todo-tree (arg)
   "Make a compact tree which shows all headlines marked with TODO.
@@ -13978,7 +13980,7 @@ The auto-repeater uses this.")
   (with-current-buffer (marker-buffer org-log-note-return-to)
     (goto-char org-log-note-return-to))
   (move-marker org-log-note-return-to nil)
-  (and org-log-post-message (message org-log-post-message)))
+  (and org-log-post-message (message "%s" org-log-post-message)))
 
 ;; FIXME: what else would be useful?
 ;; - priority
@@ -15216,10 +15218,10 @@ in the current file."
    (let* ((prop (completing-read
 		 "Property: " (org-entry-properties nil 'standard))))
      (list prop)))
-  (message (concat "Property " property
-		   (if (org-entry-delete nil property)
-		       " deleted"
-		     " was not present in the entry"))))
+  (message "Property %s %s" property
+	   (if (org-entry-delete nil property)
+	       "deleted"
+	     "was not present in the entry")))
 
 (defun org-delete-property-globally (property)
   "Remove PROPERTY globally, from all entries."
@@ -16863,7 +16865,7 @@ days in order to avoid rounding problems."
 	d (floor (+ (/ diff ds) 0.5))
 	h 0 m 0))
      (if (not to-buffer)
-	 (message (org-make-tdiff-string y d h m))
+	 (message "%s" (org-make-tdiff-string y d h m))
        (when (org-at-table-p)
 	 (goto-char match-end)
 	 (setq align t)
@@ -18806,7 +18808,7 @@ the buffer and restores the previous window configuration."
 			(org-install-agenda-files-menu)
 			(message "New agenda file list installed"))
 		      nil 'local)
-	(message (substitute-command-keys
+	(message "%s" (substitute-command-keys
 		  "Edit list and finish with \\[save-buffer]")))
     (customize-variable 'org-agenda-files)))
 
diff --git a/lisp/textmodes/reftex-index.el b/lisp/textmodes/reftex-index.el
index 73bcf6d..5b2d62c 100644
--- a/lisp/textmodes/reftex-index.el
+++ b/lisp/textmodes/reftex-index.el
@@ -367,7 +367,7 @@ _ ^        Add/Remove parent key (to make this item a subitem).
             (goto-char (or pos (point-min)))
             (or (looking-at re)
                 (reftex-nearest-match re (length literal))))
-           (t (message reftex-no-follow-message) nil))))
+           (t (message "%s" reftex-no-follow-message) nil))))
     (when match
       (goto-char (match-beginning 0))
       (recenter '(4))
diff --git a/lisp/textmodes/reftex-toc.el b/lisp/textmodes/reftex-toc.el
index d1d979b..e3d8b3e 100644
--- a/lisp/textmodes/reftex-toc.el
+++ b/lisp/textmodes/reftex-toc.el
@@ -626,7 +626,7 @@ point."
             (message "%d section%s %smoted" 
                      nsec (if (= 1 nsec) "" "s") pro-or-de)
             nil))
-    (if msg (progn (ding) (message msg)))))
+    (if msg (progn (ding) (message "%s" msg)))))
 
 
 (defun reftex-toc-restore-region (point-line &optional mark-line)
@@ -833,7 +833,7 @@ label prefix determines the wording of a reference."
                     (switch-to-buffer-other-window 
                      (reftex-get-file-buffer-force file nil))
                     (goto-char (if (eq where 'bof) (point-min) (point-max))))
-                (message reftex-no-follow-message) nil))))
+                (message "%s" reftex-no-follow-message) nil))))
 
      ((stringp (car toc))
       ;; a label
@@ -900,7 +900,7 @@ label prefix determines the wording of a reference."
                        (reftex-make-regexp-allow-for-ctrl-m literal) len)
                       (reftex-nearest-match
                        (reftex-make-desperate-section-regexp literal) len)))))
-           (t (message reftex-no-follow-message) nil))))
+           (t (message "%s" reftex-no-follow-message) nil))))
     (when match
       (goto-char (match-beginning 0))
       (if (not (= (point) (point-max))) (recenter 1))
diff --git a/lisp/textmodes/reftex.el b/lisp/textmodes/reftex.el
index 5383d88..d6ec294 100644
--- a/lisp/textmodes/reftex.el
+++ b/lisp/textmodes/reftex.el
@@ -1978,7 +1978,7 @@ Works on both Emacs and XEmacs."
   (let ((char ?\?))
     (save-window-excursion
       (catch 'exit
-        (message (concat prompt "   (?=Help)"))
+        (message "%s   (?=Help)" prompt)
         (when (or (sit-for (or delay-time 0))
                   (= ?\? (setq char (read-char-exclusive))))
           (reftex-kill-buffer "*RefTeX Select*")
@@ -1994,17 +1994,17 @@ Works on both Emacs and XEmacs."
                  (pos-visible-in-window-p (point-max)))
             nil
           (setq prompt (concat prompt (if scroll "   (SPC/DEL=Scroll)" ""))))
-        (message prompt)
+        (message "%s" prompt)
         (and (equal char ?\?) (setq char (read-char-exclusive)))
         (while t
           (cond ((equal char ?\C-g) (keyboard-quit))
                 ((equal char ?\?))
                 ((and scroll (equal char ?\ ))
                  (condition-case nil (scroll-up) (error nil))
-                 (message prompt))
+                 (message "%s" prompt))
                 ((and scroll (equal char ?\C-? ))
                  (condition-case nil (scroll-down) (error nil))
-                 (message prompt))
+                 (message "%s" prompt))
                 (t (message "") 
                    (throw 'exit char)))
           (setq char (read-char-exclusive)))))))

[-- Attachment #3: Type: text/plain, Size: 52 bytes --]



-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

[-- Attachment #4: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Fixing numerous `message' bugs..

[David Kastrup]

David Kastrup <dak@gnu.org> writes:

> How about the following (hopefully more or less exhaustive) patch?  Not
> all occurences might have the proper caution (or too much) regarding
> possible nil arguments, though.

> --- a/lisp/replace.el
> +++ b/lisp/replace.el
> @@ -1405,19 +1405,29 @@ with the `noescape' argument set.

Rats.  That one does not belong in there.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Fixing numerous `message' bugs..

[Glenn Morris]

Some mistakes in these changes are revealed by bootstrapping:


Compiling /scratch/gmorris/emacs/cvs/trunk/lisp/./ibuffer.el

In ibuffer-current-buffer:
ibuffer.el:1395:115:Warning: `error' called with 2 args to fill 1
format field(s)

Compiling /scratch/gmorris/emacs/cvs/trunk/lisp/./vc.el

In vc-update:
vc.el:2626:15:Warning: `error' called with 2 args to fill 1 format
field(s)


Compiling /scratch/gmorris/emacs/cvs/trunk/lisp/./gnus/gnus-start.el

In gnus-load:
gnus-start.el:2392:14:Warning: malformed let binding: `(error "Error
in %s line %d" file (count-lines (point-min (point))))'


Also, a changelog entry that starts:

2007-12-06  D. Goel  <deego3@gmail.com>

            * textmodes/reftex.el (reftex-TeX-master-file): Ditto.

is not very informative, IMO, when reading top-to-bottom. I would say
these should look like:

2007-12-06  D. Goel  <deego3@gmail.com>

            * textmodes/reftex.el (reftex-TeX-master-file):
            * textmodes/org.el (org-paste-subtree):
            [...]
            * allout.el (allout-write-file-hook-handler):
            Fix buggy call(s) to `error'.

Re: Fixing numerous `message' bugs..

[Dave Goel]

> Some mistakes in these changes are revealed by bootstrapping:

Thanks.  I will fix them, and bootstrap again to make sure.

Re: Fixing numerous `message' bugs..

[Johan Bockgård]

Glenn Morris <rgm@gnu.org> writes:

> Some mistakes in these changes are revealed by bootstrapping:

Others were found by eyeballing:

allout.el
gnus-art.el
reftex-parse.el
reftex.el

-- 
Johan Bockgård

Re: Fixing numerous `message' bugs..

[Glenn Morris]

Johan Bockgård wrote:

> Glenn Morris <rgm@gnu.org> writes:
>
>> Some mistakes in these changes are revealed by bootstrapping:
>
> Others were found by eyeballing:
>
> allout.el
> gnus-art.el
> reftex-parse.el
> reftex.el

Indeed. Can you be a bit more careful about this, please?
Not every instance of '(error ' is a function call.

Re: Fixing numerous `message' bugs..

[Dave Goel]

>>
>> Others were found by eyeballing:
>>
>> allout.el
>> gnus-art.el
>> reftex-parse.el
>> reftex.el
>
> Indeed. Can you be a bit more careful about this, please?
> Not every instance of '(error ' is a function call.

Thanks for having fixed these.

Re: Fixing numerous `message' bugs..

[Richard Stallman]

    +++ b/lisp/textmodes/artist.el
    @@ -1151,14 +1151,14 @@ PREV-OP-ARG are used when invoked recursively during the build-up."
       (interactive)
       (let ((next-op (cdr (cdr (assoc artist-curr-go artist-prev-next-op-alist)))))
	 (artist-select-operation next-op)
    -    (message next-op)))
    +    (message (and next-op "%s") next-op)))

I have a feeling that next-op is always supposed to be non-nil, and I
think artist-select-operation will get en error if next-op is not nil.
So I think the `and' is not needed.  How about asking the `artist.el'
maintainer?

Re: Fixing numerous `message' bugs..

[Richard Stallman]

	    (unless (current-message)
    -	  (message fancy-splash-help-echo))
    +	  (message (and fancy-splash-help-echo "%s") fancy-splash-help-echo))

I think that it is cleaner to write

    (if fancy-splash-help-echo (message "%s" fancy-splash-help-echo)
      (message nil))

Re: Fixing numerous `message' bugs..

[Richard Stallman]

Here's an idea: make up an argument to `message' that says treat the
next argument literally.  How about t?

   (message t STRING) is equivalent to (message "%s" STRING)
   (message t nil) is equivalent to (message nil)

So in the cases that are buggy it suffices to add t as the first argument.

Re: Fixing numerous `message' bugs..

[Richard Stallman]

    (message 
	    (if foo
		    nil
		    (if bar "abc"
			    (concat "def" filename))))

I think the right fix for that is

   (if foo (message nil)
     (message "%s" (if bar "abc" (concat "def" filename))))

or

   (if foo (message nil)
     (message (if bar "abc" "def%s")
               filename))

I think either of those is clearer than the original code, and would
be cleaner than the fix using the proposed `msg' function.

Re: Fixing numerous `message' bugs..

[David Kastrup]

Richard Stallman <rms@gnu.org> writes:

> Here's an idea: make up an argument to `message' that says treat the
> next argument literally.  How about t?
>
>    (message t STRING) is equivalent to (message "%s" STRING)
>    (message t nil) is equivalent to (message nil)
>
> So in the cases that are buggy it suffices to add t as the first argument.

I don't particularly like it.  What about (message t nil 7), what is
that supposed to return?  nil too?  Anyway, there is little point to
make this specific to message.  So if at all, we would special-case
format instead.  Which means that (stringp (format ...)) is no longer
guaranteed to be true.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Fixing numerous `message' bugs..

[David Kastrup]

Richard Stallman <rms@gnu.org> writes:

>     +++ b/lisp/textmodes/artist.el
>     @@ -1151,14 +1151,14 @@ PREV-OP-ARG are used when invoked recursively during the build-up."
>        (interactive)
>        (let ((next-op (cdr (cdr (assoc artist-curr-go artist-prev-next-op-alist)))))
> 	 (artist-select-operation next-op)
>     -    (message next-op)))
>     +    (message (and next-op "%s") next-op)))
>
> I have a feeling that next-op is always supposed to be non-nil, and I
> think artist-select-operation will get en error if next-op is not nil.
> So I think the `and' is not needed.  How about asking the `artist.el'
> maintainer?

Well, I was going through this sort of defensively.  If I couldn't
figure out in 10 seconds whether the string could contain percent signs,
I went with "%s", and if I couldn't figure out in the same time frame
whether it could be nil, I added the "and".

It would probably be cleaner to omit the "and" when in doubt and let the
package maintainers clean up afterwards in the rare case where this was
mistaken.  It is also possible to write this as

(message "%s" (concat next-op))

which is more concise and avoids double evaluation of next-op.  It does
copy next-op, though.  So perhaps

(message "%s" (or next-op ""))

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Fixing numerous `message' bugs..

[Dave Goel]

>
>> Here's an idea: make up an argument to `message' that says treat the
>> next argument literally.  How about t?
>>
>>    (message t STRING) is equivalent to (message "%s" STRING)
>>    (message t nil) is equivalent to (message nil)
>>
>> So in the cases that are buggy it suffices to add t as the first argument.

The above is the neatest idea so far.  It does not involve a new
function, and makes fixing code really easy.  And, any author that desires
a literal treatment can simply use (message t string).

> I don't particularly like it.  What about (message t nil 7), what is
> that supposed to return?  nil too? 

Sure, why not?  ((Or, it could return an error. ))

> Anyway, there is little point to make this specific to message.  So
> if at all, we would special-case format instead.  Which means that
> (stringp (format ...)) is no longer guaranteed to be true.


Can you elaborate?  Why should a `message' bug cause changes to
something as general as format?  And, how will that fix the problem at hand?

Besides, tweaking `format' will further our non-compatibility with
Common Lisp.

Re: Fixing numerous `message' bugs..

[Stefan Monnier]

> And, any author that desires a literal treatment can simply use
> (message t string).

The problem is that all those bugs are here because the corresponding
authors just didn't think, so they are unlikely to use this new
arg either.


        Stefan

Re: Fixing numerous `message' bugs..

[Glenn Morris]

Stefan Monnier wrote:

> The problem is that all those bugs are here because the corresponding
> authors just didn't think, so they are unlikely to use this new
> arg either.

Is there any way to improve byte-compile-format-warn in this regard?

It's easy to make it warn if the first arg to format is not a fixed
string or nil, but that leaves false positives for things like:

  (setq fmt "%s")
  (message fmt some-variable)

Can the byte-compiler figure out what is a known constant at compile
time? One would only want it to warn where the first argument is not
fully known at compile-time.

Because everyone pays attention to compiler warnings, right? ;)

Re: Fixing numerous `message' bugs..

[Stefan Monnier]

>> The problem is that all those bugs are here because the corresponding
>> authors just didn't think, so they are unlikely to use this new
>> arg either.

> Is there any way to improve byte-compile-format-warn in this regard?

Kind of, but it's not very satifactory.  In contrast, if we change
(message FOO) to behave like (if FOO (message "%s" FOO) (message nil)),
then naive coders will simply not bump into any surprise, even if they
don't look at byte-compiler warnings.  Also it will save people from
quoting the % char in those cases.
The only downside is the need to change existing code.


        Stefan

Re: Fixing numerous `message' bugs..

[David Kastrup]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> The problem is that all those bugs are here because the corresponding
>>> authors just didn't think, so they are unlikely to use this new
>>> arg either.
>
>> Is there any way to improve byte-compile-format-warn in this regard?
>
> Kind of, but it's not very satifactory.  In contrast, if we change
> (message FOO) to behave like (if FOO (message "%s" FOO) (message nil)),
> then naive coders will simply not bump into any surprise, even if they
> don't look at byte-compiler warnings.  Also it will save people from
> quoting the % char in those cases.
> The only downside is the need to change existing code.

Yes.  There is indeed code using "%%" in a single argument.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Fixing numerous `message' bugs..

[Richard Stallman]

    I don't particularly like it.  What about (message t nil 7), what is
    that supposed to return?  nil too?

The same as (message nil 7) returns now.
(The value is nil.)

I don't see that this is a problem.

      Anyway, there is little point to
    make this specific to message.  So if at all, we would special-case
    format instead.

I don't see why.  There is no problem or issue with `format'.  The
issue arises with `message' because (message nil) is meaningful.

Re: Fixing numerous `message' bugs..

[Richard Stallman]

    > And, any author that desires a literal treatment can simply use
    > (message t string).

    The problem is that all those bugs are here because the corresponding
    authors just didn't think, so they are unlikely to use this new
    arg either.

The issue is not directly about what anyone is likely to do.
The issue is how to make it easy to fix these things.

I agree it would be nice if this were so clear that nobody
would ever do it wrong, but that may be too much to ask.

Re: Fixing numerous `message' bugs..

[Dave Goel]

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



[...]


I am a C newbie.  I was playing around with editfns.c that would
implement RMS's (message t ..) idea.. , and tried the attached patch.
It seemed to compile fine.  But, compiling via 'make bootstrap' always
fails at the end with a segfault, as seen below.  If I revert to the
original editfns.c, 'make bootstrap' finishes all the way to the end.
Any hints what I am doing wrong?  Thanks.


Loading loadup.el (source)...
(/home/deego/emacscvs-anon/emacs/lisp)
Loading emacs-lisp/byte-run...
Loading emacs-lisp/backquote...
Loading subr...
Loading version.el (source)...
Loading widget...
Loading custom...
Loading emacs-lisp/map-ynp...
Loading cus-start...
Loading international/mule...
Loading international/mule-conf.el (source)...
Loading env...
Loading format...
Loading bindings...
Loading files...
Loading cus-face...
Loading faces...
Loading button...
Loading startup...
make[3]: *** [emacs] Segmentation fault
make[3]: Leaving directory

make[2]: *** [src] Error 2




[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: editfnsdiff200711.c --]
[-- Type: text/x-csrc, Size: 1368 bytes --]

--- editfns.c	2007-12-10 11:56:42.000000000 -0500
+++ editfnsmy.c	2007-12-10 11:56:34.000000000 -0500
@@ -3221,25 +3221,40 @@
 any existing message; this lets the minibuffer contents show.  See
 also `current-message'.
 
+If the first argument is t, the function is equivalent to calling 
+(message "%s" second-argument).
+
 usage: (message FORMAT-STRING &rest ARGS)  */)
      (nargs, args)
      int nargs;
      Lisp_Object *args;
 {
-  if (NILP (args[0])
-      || (STRINGP (args[0])
-	  && SBYTES (args[0]) == 0))
+  if (Fequal(args[0],Qt))
     {
-      message (0);
-      return args[0];
+      register Lisp_Object val; 
+      Lisp_Object argb[2]; 
+      argb[0]=build_string("%s"); 
+      argb[1]=args[1]; 
+      val = Fformat (2, argb); 
+      message3 (val, SBYTES (val), STRING_MULTIBYTE (val)); 
+      return 1;
     }
   else
     {
-      register Lisp_Object val;
-      val = Fformat (nargs, args);
-      message3 (val, SBYTES (val), STRING_MULTIBYTE (val));
-      return val;
-    }
+      if (NILP (args[0])
+	  || (STRINGP (args[0])
+	      && SBYTES (args[0]) == 0))
+	{
+	  message (0);
+	  return args[0];
+	}
+      else
+	{
+	  register Lisp_Object val;
+	  val = Fformat (nargs, args);
+	  message3 (val, SBYTES (val), STRING_MULTIBYTE (val));
+	  return val;
+	}}
 }
 
 DEFUN ("message-box", Fmessage_box, Smessage_box, 1, MANY, 0,

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Fixing numerous `message' bugs..

[Jason Rumney]

Dave Goel wrote:
> [...]
>
>
> I am a C newbie.  I was playing around with editfns.c that would
> implement RMS's (message t ..) idea.. , and tried the attached patch.
> It seemed to compile fine.  But, compiling via 'make bootstrap' always
> fails at the end with a segfault, as seen below.  If I revert to the
> original editfns.c, 'make bootstrap' finishes all the way to the end.
> Any hints what I am doing wrong?  Thanks.
>   
You're returning 1, where a Lisp_Object is expected.

I'd refactor your changes to the following, to make it clearer what is
actually being changed (most of the diff you posted is whitespace
changes due to increased nesting):

   if (NILP (args[0])
     {
       message (0);
       return args[0];
     }
+  else if (Fequal(args[0],Qt))
+    {
+      register Lisp_Object val;
+      Lisp_Object argb[2];
+      argb[0]=build_string("%s");
+      argb[1]=args[1];
+      val = Fformat (2, argb);
+      message3 (val, SBYTES (val), STRING_MULTIBYTE (val));
+      return 1;
+    }
   else
     {
       register Lisp_Object val;
       val = Fformat (nargs, args);

Re: Fixing numerous `message' bugs..

[Dave Goel]

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

>>   
> You're returning 1, where a Lisp_Object is expected.

Thanks.  I corrected it to return val, but still get the same segfault
upon "make bootstrap".  Attached is the current diff, but it is much
easier to simply read the new function I am trying -- 



DEFUN ("message", Fmessage, Smessage, 1, MANY, 0,
       doc: /* Display a message at the bottom of the screen.
.
.
usage: (message FORMAT-STRING &rest ARGS)  */)
     (nargs, args)
     int nargs;
     Lisp_Object *args;
{
  if (Fequal(args[0],Qt))
  {
    register Lisp_Object val; 
    Lisp_Object argb[2]; 
    argb[0]=build_string ("%s"); 
    argb[1]=args[1]; 
    val = Fformat (2, argb); 
    message3 (val, SBYTES (val), STRING_MULTIBYTE (val)); 
    return val;
  }
  else
    {
      if (NILP (args[0])
	  || (STRINGP (args[0])
	      && SBYTES (args[0]) == 0))
	{
	  message (0);
	  return args[0];
	}
      else
	{
	  register Lisp_Object val;
	  val = Fformat (nargs, args);
	  message3 (val, SBYTES (val), STRING_MULTIBYTE (val));
	  return val;
	}
    }
}


The error remains:
.
Loading loadup.el (source)...
.
Loading emacs-lisp/byte-run...
.
.
Loading format...
Loading bindings...
Loading files...
Loading cus-face...
Loading faces...
Loading button...
Loading startup...
make[3]: *** [emacs] Segmentation fault
make[3]: Leaving directory
..
make[2]: *** [src] Error 2



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: editfnsdiff200711.c --]
[-- Type: text/x-csrc, Size: 1380 bytes --]

--- editfns.c	2007-12-10 11:56:42.000000000 -0500
+++ editfnsmy.c.200711	2007-12-10 13:59:36.000000000 -0500
@@ -3221,25 +3221,40 @@
 any existing message; this lets the minibuffer contents show.  See
 also `current-message'.
 
+If the first argument is t, the function is equivalent to calling 
+(message "%s" second-argument).
+
 usage: (message FORMAT-STRING &rest ARGS)  */)
      (nargs, args)
      int nargs;
      Lisp_Object *args;
 {
-  if (NILP (args[0])
-      || (STRINGP (args[0])
-	  && SBYTES (args[0]) == 0))
+  if (Fequal(args[0],Qt))
     {
-      message (0);
-      return args[0];
+      register Lisp_Object val; 
+      Lisp_Object argb[2]; 
+      argb[0]=build_string ( "%s" ); 
+      argb[1]=args[1]; 
+      val = Fformat (2, argb); 
+      message3 (val, SBYTES (val), STRING_MULTIBYTE (val)); 
+      return val;
     }
   else
     {
-      register Lisp_Object val;
-      val = Fformat (nargs, args);
-      message3 (val, SBYTES (val), STRING_MULTIBYTE (val));
-      return val;
-    }
+      if (NILP (args[0])
+	  || (STRINGP (args[0])
+	      && SBYTES (args[0]) == 0))
+	{
+	  message (0);
+	  return args[0];
+	}
+      else
+	{
+	  register Lisp_Object val;
+	  val = Fformat (nargs, args);
+	  message3 (val, SBYTES (val), STRING_MULTIBYTE (val));
+	  return val;
+	}}
 }
 
 DEFUN ("message-box", Fmessage_box, Smessage_box, 1, MANY, 0,

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Fixing numerous `message' bugs..

[Andreas Schwab]

Dave Goel <deego3@gmail.com> writes:

>   if (Fequal(args[0],Qt))

Fequal returns a Lisp object, either Qt or Qnil, both of which are
always non-zero.  To convert it to a boolean you need to use NILP
(compiling with USE_LISP_UNION_TYPE helps finding such bugs).  But you
want to use EQ instead anyway, since you are comparing with a symbol.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Fixing numerous `message' bugs..

[Dave Goel]

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

Andreas Schwab <schwab@suse.de> writes:


> want to use EQ instead anyway, since you are comparing with a symbol.

Thanks.  Attached is a new patch, which also seems much more elegant.  
I am not a C expert.  Can some one comment on it?  Shall I commit it?

I have tested it in these ways, and it seems to implement the
suggested functionality completely.

(message t)  
nil

(message t nil) 
nil

(message t "%s") 
"%s"

We changed args[0], Is the value intact? -- 

(let ((a t))
  (message a)
  a)    

t


(message t "%%")
"%%"

(message "%%")
"%"


(message t nil 1 2 3 4 ) 
nil



(message t "a" 1 2 3 4 ) 
"a"



(message "%s" 1 2 3 4 ) 
"1"




----


[-- Attachment #2: editfns.c.diff200711 --]
[-- Type: application/octet-stream, Size: 726 bytes --]

--- backup/editfns-20071123-0825-23---20071210-1135-07.c	2007-11-23 08:25:23.000000000 -0500
+++ editfns.c	2007-12-10 15:23:51.000000000 -0500
@@ -3221,11 +3221,21 @@
 any existing message; this lets the minibuffer contents show.  See
 also `current-message'.
 
+If the first argument is t, the function is equivalent to calling
+(message "%s" second-argument).
+
 usage: (message FORMAT-STRING &rest ARGS)  */)
      (nargs, args)
      int nargs;
      Lisp_Object *args;
 {
+  /* check if args0==t */
+  if (!NILP(Feq(args[0],Qt)))
+    if (NILP (args[1]) || (nargs==1))
+      args[0]=Qnil;
+    else
+      args[0]=build_string("%s");
+  
   if (NILP (args[0])
       || (STRINGP (args[0])
 	  && SBYTES (args[0]) == 0))

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Fixing numerous `message' bugs..

[David Kastrup]

Dave Goel <deego3@gmail.com> writes:

> Andreas Schwab <schwab@suse.de> writes:
>
>
>> want to use EQ instead anyway, since you are comparing with a symbol.
>
> Thanks.  Attached is a new patch, which also seems much more elegant.
> I am not a C expert.  Can some one comment on it?  Shall I commit it?

Considering that the idiom

(message "%s" (or x ""))

will work instead of the proposed (message t x) and considering that it
rarely occurs (I think I should have got most cases), I don't think it
is really worth the trouble.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Fixing numerous `message' bugs..

[Dave Goel]

> (message "%s" (or x ""))
>
> will work instead of the proposed (message t x) 

(a) This looks like a neat "trick", but is confusing code.  Code
should be readable and as manifest as possible, and not rely on such
"hidden" tricks.  On the obfuscation scale, this is far worse than
something as innocuous as (or foo (error bar)), and note that some
authors even oppose that..

(b) This code also relies on an undocumented feature of `message' - that
"" as the first argument makes it behave almost the same as if the
argument were nil.


(c) Also note that (message "") is not exactly the same as (message
nil), because they return different values.  

> and considering that it rarely occurs (I think I should have got
> most cases),

Most, but not all.  

> I don't think it is really worth the trouble.

Hm, but where is the trouble?  This is a backward-compatible change.
After all, it doesn't change a thing for those that are well-versed in
the proper use of `message' and always remember to use the first
argument as a format string.

I, for one, still think instinctively think of (message) and (error)
as if they called their strings literally.  It is obvious that many
others do too.  Perhaps, (message t ..) would slowly become a habit
for such authors looking for literal strings, just as (format nil .. )
is a habit for common-lispers...  

In fact, I would prefer such a functionality for `error' as well..

Re: Fixing numerous `message' bugs..

[David Kastrup]

Dave Goel <deego3@gmail.com> writes:

>> (message "%s" (or x ""))
>>
>> will work instead of the proposed (message t x) 
>
> (a) This looks like a neat "trick", but is confusing code.  Code
> should be readable and as manifest as possible, and not rely on such
> "hidden" tricks.  On the obfuscation scale, this is far worse than
> something as innocuous as (or foo (error bar)), and note that some
> authors even oppose that..
>
> (b) This code also relies on an undocumented feature of `message' - that
> "" as the first argument makes it behave almost the same as if the
> argument were nil.

Undocumented?


    If the first argument is nil or the empty string, the function clears
    any existing message; this lets the minibuffer contents show.  See
    also `current-message'.

It is not the first argument, though, that is "" here.

> (c) Also note that (message "") is not exactly the same as (message
> nil), because they return different values.  
>
>> and considering that it rarely occurs (I think I should have got
>> most cases),
>
> Most, but not all.

Which one is missing?  Note that you still have to hunt all of them
down.

>> I don't think it is really worth the trouble.
>
> Hm, but where is the trouble?  This is a backward-compatible change.

But it is a surprising one, inconsistent with `format'.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum