safe_call1 considered harmful

safe_call1 considered harmful

[Eli Zaretskii]

The change below was presumably made to avoid errors in functions that
are put on the various *-coding-system-alist variables.  Those errors
might have been caused by the recent changes in several modes that now
use a cons cell `(FILENAME . BUFFER)' instead of just the file name as
the argument to find-operation-coding-system (when the operation is
insert-file-contents), because some packages put functions on the
file-coding-system-alist that are not ready for the cons cell.

I think the change in coding.c is for the worse: it masks such
problems from us, so instead of seeing bug reports, we sweep the
problems under the carpet, where they run risk to be left undetected
until after the release.

A case in point is the function find-buffer-file-type-coding-system
that dos-w32.el adds to file-coding-system-alist: it was not modified
to support the change in the find-operation-coding-system's interface,
and caused files with DOS EOLs uncompressed from archives to be shown
with the ^M characters.  This happened because
find-buffer-file-type-coding-system throws an error, but safe_call1
silently ignores it.

So how about if we undo the change below?


2006-05-29  Kenichi Handa  <handa@m17n.org>

	* coding.c (Ffind_operation_coding_system): Call a function by
	safe_call1 instead of call1.

Index: src/coding.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/coding.c,v
retrieving revision 1.338
retrieving revision 1.339
diff -u -r1.338 -r1.339
--- src/coding.c	26 May 2006 04:48:21 -0000	1.338
+++ src/coding.c	29 May 2006 00:54:27 -0000	1.339
@@ -7539,7 +7539,7 @@
 	    return Fcons (val, val);
 	  if (! NILP (Ffboundp (val)))
 	    {
-	      val = call1 (val, Flist (nargs, args));
+	      val = safe_call1 (val, Flist (nargs, args));
 	      if (CONSP (val))
 		return val;
 	      if (SYMBOLP (val) && ! NILP (Fcoding_system_p (val)))

Re: safe_call1 considered harmful

[Kenichi Handa]

In article <uirlrguz3.fsf@gnu.org>, Eli Zaretskii <eliz@gnu.org> writes:

> The change below was presumably made to avoid errors in functions that
> are put on the various *-coding-system-alist variables.  Those errors
> might have been caused by the recent changes in several modes that now
> use a cons cell `(FILENAME . BUFFER)' instead of just the file name as
> the argument to find-operation-coding-system (when the operation is
> insert-file-contents), because some packages put functions on the
> file-coding-system-alist that are not ready for the cons cell.

> I think the change in coding.c is for the worse: it masks such
> problems from us, so instead of seeing bug reports, we sweep the
> problems under the carpet, where they run risk to be left undetected
> until after the release.

> A case in point is the function find-buffer-file-type-coding-system
> that dos-w32.el adds to file-coding-system-alist: it was not modified
> to support the change in the find-operation-coding-system's interface,
> and caused files with DOS EOLs uncompressed from archives to be shown
> with the ^M characters.  This happened because
> find-buffer-file-type-coding-system throws an error, but safe_call1
> silently ignores it.

> So how about if we undo the change below?

Ah, hmmm, I didn't think about such a situation.  The change
was to avoid the backward incompatiblity reported by the
attached mail.

But, by considering this problem again, I found another
solution than calling find-operation-coding-system with
(FILENAME . BUFFER).  That is to provide an extra argument
BUFFER.  Then, we can keep backward compatibility and
find-buffer-file-type-coding-system works as before, and, by
modifying po-find-file-coding-system to check that extra
argument instead of checking if FILENAME is cons or not, we
can make it work well too.

Do I still miss something?  If not, I'll try to change the
current code along that line.

---
Kenichi Handa
handa@m17n.org

Date: Sun, 28 May 2006 10:23:16 +0200
From: Sven Joachim <svenjoac@gmx.de>
MIME-Version: 1.0
To: Kenichi Handa <handa@m17n.org>
In-Reply-To: <E1FkDtH-0002qI-00@etlken>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Cc: emacs-pretest-bug@gnu.org
Subject: Re: Coding system of compressed PO files is not recognized
X-Spam-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00 autolearn=ham 	version=3.0.3

Kenichi Handa wrote:
> Do you mean that, po-compat.el was able to detect a coding
> system of compressed PO file correctly before my change?

No, it wasn't.  But at least it was able to visit the file
without getting an error. ;-)



_______________________________________________
emacs-pretest-bug mailing list
emacs-pretest-bug@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-pretest-bug

Re: safe_call1 considered harmful

[Eli Zaretskii]

> From: Kenichi Handa <handa@m17n.org>
> CC: emacs-devel@gnu.org
> Date: Fri, 21 Jul 2006 20:34:16 +0900
> 
> But, by considering this problem again, I found another
> solution than calling find-operation-coding-system with
> (FILENAME . BUFFER).  That is to provide an extra argument
> BUFFER.  Then, we can keep backward compatibility and
> find-buffer-file-type-coding-system works as before, and, by
> modifying po-find-file-coding-system to check that extra
> argument instead of checking if FILENAME is cons or not, we
> can make it work well too.

This will work if no function on file-coding-system-alist currently
looks at arguments beyond the 1st one, the file name.  If there are
functions which look beyond that, such a change will break them.
While the chance of having such functions is very small, I don't see
how any significant change in the API could avoid breaking some
function that wasn't written to support the new API.

In any case, I think we should revert the change you made to use
safe_call1.  Then we will quickly find any remaining functions that
need to be modified.

Re: safe_call1 considered harmful

[Kenichi Handa]

In article <uac73gdpy.fsf@gnu.org>, Eli Zaretskii <eliz@gnu.org> writes:

>> But, by considering this problem again, I found another
>> solution than calling find-operation-coding-system with
>> (FILENAME . BUFFER).  That is to provide an extra argument
>> BUFFER.  Then, we can keep backward compatibility and
>> find-buffer-file-type-coding-system works as before, and, by
>> modifying po-find-file-coding-system to check that extra
>> argument instead of checking if FILENAME is cons or not, we
>> can make it work well too.

> This will work if no function on file-coding-system-alist currently
> looks at arguments beyond the 1st one, the file name.  If there are
> functions which look beyond that, such a change will break them.

No, what I meant is to give the extra argument BUFFER at the
END of the normal arguments to insert-file-contents.  So,
for instance, tar-extract will call
find-operation-coding-system as this:

			     (car (find-operation-coding-system
				   'insert-file-contents
				   name t nil nil nil (current-buffer)))

The new docstring (only the last paragraph) for
find-operation-coding-system will be:

----------------------------------------------------------------------
[...]
If OPERATION is `insert-file-contents', there may be an extra argument
BUFFER at the end.  In that case, FILENAME is a file name to look up,
and BUFFER is a buffer that contains the file's contents (not yet
decoded).  If `file-coding-system-alist' specifies a function to call
for FILENAME, that function should examine the contents of BUFFER
instead of reading the file.
----------------------------------------------------------------------

This change breaks only such a function that checks that the
number of given arguments is not greater than 5 (the maximum
arguments to insert-file-contents).  I believe such a
function is very very rare.

> In any case, I think we should revert the change you made to use
> safe_call1.

Yes.

---
Kenichi Handa
handa@m17n.org

Re: safe_call1 considered harmful

[Eli Zaretskii]

> From: Kenichi Handa <handa@m17n.org>
> CC: emacs-devel@gnu.org
> Date: Mon, 24 Jul 2006 10:36:12 +0900
> 
> > In any case, I think we should revert the change you made to use
> > safe_call1.
> 
> Yes.

Done.

Re: safe_call1 considered harmful

[Kenichi Handa]

In article <u1ws4d6b1.fsf@gnu.org>, Eli Zaretskii <eliz@gnu.org> writes:

>> > In any case, I think we should revert the change you made to use
>> > safe_call1.
>> 
>> Yes.

> Done.

Thank you.

---
Kenichi Handa
handa@m17n.org

Re: safe_call1 considered harmful

[Richard Stallman]

    But, by considering this problem again, I found another
    solution than calling find-operation-coding-system with
    (FILENAME . BUFFER).  That is to provide an extra argument
    BUFFER.

Would you please be more concrete?  Passing an extra argument to a set
of functions is very incompatible.

Re: safe_call1 considered harmful

[Kenichi Handa]

In article <E1G40o4-0007uN-FH@fencepost.gnu.org>, Richard Stallman <rms@gnu.org> writes:

>     But, by considering this problem again, I found another
>     solution than calling find-operation-coding-system with
>     (FILENAME . BUFFER).  That is to provide an extra argument
>     BUFFER.

> Would you please be more concrete?  Passing an extra argument to a set
> of functions is very incompatible.

find-operation-coding-system calls a registered function
with a single argument; a list of arguments given to
find-operation-coding-system.  So, appending an extra
argument at the tail of the list is fairly safe.

---
Kenichi Handa
handa@m17n.org

Re: safe_call1 considered harmful

[Richard Stallman]

    find-operation-coding-system calls a registered function
    with a single argument; a list of arguments given to
    find-operation-coding-system.  So, appending an extra
    argument at the tail of the list is fairly safe.

It might seem that way; but what happens if we change the calling
convention of one of the operations, giving it an additional argument?
That may be necessary some day.  I prefer the current convention.

Re: safe_call1 considered harmful

[Kenichi Handa]

In article <E1G5548-0002HD-8x@fencepost.gnu.org>, Richard Stallman <rms@gnu.org> writes:

>     find-operation-coding-system calls a registered function
>     with a single argument; a list of arguments given to
>     find-operation-coding-system.  So, appending an extra
>     argument at the tail of the list is fairly safe.

> It might seem that way; but what happens if we change the calling
> convention of one of the operations, giving it an additional argument?
> That may be necessary some day.  I prefer the current convention.

We can change tar-mode/arc-mode/jka-compr to call
find-operation-coding-system with full number of arguments
plus the extra argument BUFFER.  Then, a function called
from find-operation-coding-system can check if the number of
arguments is greater than the normal number (which can be
checked by subr-arity).  If it is greater, the function can
know that the last argument is BUFFER.  By that way, the
function doesn't break even if we add a new argument to
find-operation-coding-system.

---
Kenichi Handa
handa@m17n.org

Re: safe_call1 considered harmful

[Richard Stallman]

    We can change tar-mode/arc-mode/jka-compr to call
    find-operation-coding-system with full number of arguments
    plus the extra argument BUFFER.

I'd rather leave it alone.

Re: safe_call1 considered harmful

[Kenichi Handa]

In article <E1G7PXw-0000FA-Js@fencepost.gnu.org>, Richard Stallman <rms@gnu.org> writes:

>     We can change tar-mode/arc-mode/jka-compr to call
>     find-operation-coding-system with full number of arguments
>     plus the extra argument BUFFER.

> I'd rather leave it alone.

If you think the backward incompatible problem raised by the
current code is not that a big problem, I don't argue
anymore.  Shall I install this change in etc/NEWS?

*** NEWS	31 Jul 2006 09:41:07 +0900	1.1382
--- NEWS	31 Jul 2006 14:08:58 +0900	
***************
*** 3661,3668 ****
  \f
  * Incompatible Lisp Changes in Emacs 22.1
  
! ** The function find-operation-coding-system accepts a cons (FILENAME
! . BUFFER) in an argument correponding to the target.
  
  ---
  ** The variables post-command-idle-hook and post-command-idle-delay have
--- 3661,3670 ----
  \f
  * Incompatible Lisp Changes in Emacs 22.1
  
! ** The function find-operation-coding-system may be called with a cons
! (FILENAME . BUFFER) in the second argument if the first argument
! OPERATION is `insert-file-contents', and thus a function registered in
! `file-coding-system-alist' is also called with such an argument.
  
  ---
  ** The variables post-command-idle-hook and post-command-idle-delay have

---
Kenichi Handa
handa@m17n.org

Re: safe_call1 considered harmful

[Richard Stallman]

    If you think the backward incompatible problem raised by the
    current code is not that a big problem, I don't argue
    anymore.  Shall I install this change in etc/NEWS?

Please do.

Re: safe_call1 considered harmful

[Kenichi Handa]

Richard Stallman <rms@gnu.org> writes:

>     If you think the backward incompatible problem raised by the
>     current code is not that a big problem, I don't argue
>     anymore.  Shall I install this change in etc/NEWS?

> Please do.

Done.

---
Kenichi Handa
handa@m17n.org