string-to-unibyte in image-jpeg-p

string-to-unibyte in image-jpeg-p

[Thien-Thi Nguyen]

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


One of the last ‘string-to-unibyte’ (deprecated for a while)
calls is in ‘image-jpeg-p’:

 (defun image-jpeg-p (data)
   "..."
   (setq data (ignore-errors (string-to-unibyte data)))
   ...)

We can simulate ‘string-to-unibyte’ (except for error message
particulars) with ‘FAKE-string-to-unibyte’, which uses the
modern ‘encode-coding-string’:

 (defun FAKE-string-to-unibyte (s)
   (let ((tem (encode-coding-string s 'binary)))
     (when (string-match-p "\xc2" tem)
       (error "badness"))
     tem))

However, the original call ignores errors, so this:

 (setq data (ignore-errors (FAKE-string-to-unibyte data)))

can be simplified to this:

 (setq data (FAKE/NOERROR-string-to-unibyte data))

by defining:

 (defun FAKE/NOERROR-string-to-unibyte (s)
   (let ((tem (encode-coding-string s 'binary)))
     (unless (string-match-p "\xc2" tem)
       tem)))

The last step is to simply inline the definition:

 (setq data (let ((tem (encode-coding-string data 'binary)))
              (unless (string-match-p "\xc2" tem)
                tem)))

My questions are:
- Is my reasoning correct?  In particular, i'd like to confirm
  that the "error" detection via "\xc2" is a valid strategy.
- If so, which branch gets the change, ‘emacs-26’ or ‘master’?
- If not, what am i missing?

-- 
Thien-Thi Nguyen -----------------------------------------------
 (defun responsep (query)
   (pcase (context query)
     (`(technical ,ml) (correctp ml))
     ...))                              748E A0E8 1CB8 A748 9BFA
--------------------------------------- 6CE4 6703 2224 4C80 7502


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: string-to-unibyte in image-jpeg-p

[Stefan Monnier]

> One of the last ‘string-to-unibyte’ (deprecated for a while)
> calls is in ‘image-jpeg-p’:
>
>  (defun image-jpeg-p (data)
>    "..."
>    (setq data (ignore-errors (string-to-unibyte data)))
>    ...)

To figure out the best fix, we need to know:
- Why are errors ignored?
- Why/when is data a multibyte string?

> We can simulate ‘string-to-unibyte’ (except for error message
> particulars) with ‘FAKE-string-to-unibyte’, which uses the
> modern ‘encode-coding-string’:
>
>  (defun FAKE-string-to-unibyte (s)
>    (let ((tem (encode-coding-string s 'binary)))
>      (when (string-match-p "\xc2" tem)
>        (error "badness"))

You can't really detect a "non-ascii non-rawbyte input" by looking at
the output of encode-coding-string, so this is not the right approach.

You could try something like:

    (unless (unibyte-string-p data)
      (setq data
            ;; \x3fffnn encode "raw bytes" in multibyte strings.
            (if (string-match "[^\x00-\xff\x3fff80-\x3fffff]" data)
                nil
              (encode-coding-string data 'binary))))

> - If so, which branch gets the change, ‘emacs-26’ or ‘master’?

Definitely not emacs-26


        Stefan

Re: string-to-unibyte in image-jpeg-p

[Eli Zaretskii]

> From: Thien-Thi Nguyen <ttn@gnu.org>
> Date: Wed, 23 May 2018 12:21:52 +0200
> 
>  (setq data (let ((tem (encode-coding-string data 'binary)))
>               (unless (string-match-p "\xc2" tem)
>                 tem)))
> 
> My questions are:
> - Is my reasoning correct?  In particular, i'd like to confirm
>   that the "error" detection via "\xc2" is a valid strategy.

I'm not sure.  #xC2 is the leading byte of a 5-byte representation of
raw bytes, right?  There are also 2-byte representations, which begin
with C0 and C1 respectively.

> - If so, which branch gets the change, ‘emacs-26’ or ‘master’?

master in any case, but see below.

> - If not, what am i missing?

Doesn't it feel strange to encode a string using 'binary', which is an
alias for 'no-conversion' as the encoding?  Is it intuitive to do
something like that?  Is the meaning obvious (I guess not)?

So I'd rather we left string-to-unibyte alone in this case, instead of
artificially replacing it with encode-coding-string.  And maybe we
should un-deprecate string-to-unibyte, since this is the _only_ place
where it's called, and if used correctly, that function has no good
replacements.

Thanks.

Re: string-to-unibyte in image-jpeg-p

[Andreas Schwab]

On Mai 23 2018, Thien-Thi Nguyen <ttn@gnu.org> wrote:

> - If not, what am i missing?

Given that image-jpeg-p is no longer used, perhaps it should just be
removed?

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: string-to-unibyte in image-jpeg-p

[Stefan Monnier]

>> - If not, what am i missing?
> Given that image-jpeg-p is no longer used, perhaps it should just be
> removed?

That would be even better!
How do we know it's not used any more, tho?


        Stefan

Re: string-to-unibyte in image-jpeg-p

[Paul Eggert]

On 05/23/2018 01:29 PM, Stefan Monnier wrote:
> How do we know it's not used any more, tho?

We deprecate it and wait for nobody to complain.

Re: string-to-unibyte in image-jpeg-p

[Robert Pluim]

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 05/23/2018 01:29 PM, Stefan Monnier wrote:
>> How do we know it's not used any more, tho?
>
> We deprecate it and wait for nobody to complain.

The comments in image.el say:

;; Used to be in image-type-header-regexps, but now not used anywhere
;; (since 2009-08-28).

so it was an internal function in any case, so I think itʼs fair game
to just remove it.

Robert

Re: string-to-unibyte in image-jpeg-p

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Thu, 24 May 2018 10:51:01 +0200
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
> 
> Paul Eggert <eggert@cs.ucla.edu> writes:
> 
> > On 05/23/2018 01:29 PM, Stefan Monnier wrote:
> >> How do we know it's not used any more, tho?
> >
> > We deprecate it and wait for nobody to complain.
> 
> The comments in image.el say:
> 
> ;; Used to be in image-type-header-regexps, but now not used anywhere
> ;; (since 2009-08-28).
> 
> so it was an internal function in any case, so I think itʼs fair game
> to just remove it.

Maybe so, but I prefer Paul's way.  We should be very careful not to
remove features hastily, without due heads-up to possible users.

Re: string-to-unibyte in image-jpeg-p

[Robert Pluim]

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

Eli Zaretskii <eliz@gnu.org> writes:

> Maybe so, but I prefer Paul's way.  We should be very careful not to
> remove features hastily, without due heads-up to possible users.

Ok for master?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Obsolete-image-jpeg-p.patch --]
[-- Type: text/x-diff, Size: 692 bytes --]

From 5b7f50a8a7358b39119fecd8706f5c1713421379 Mon Sep 17 00:00:00 2001
From: Robert Pluim <rpluim@gmail.com>
Date: Thu, 24 May 2018 17:28:11 +0200
Subject: [PATCH] Obsolete image-jpeg-p
To: emacs-devel@gnu.org

* lisp/image.el (image-jpeg-p): Declare obsolete.
---
 lisp/image.el | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lisp/image.el b/lisp/image.el
index e084fe329a..95acda0b07 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -270,6 +270,8 @@ image-jpeg-p
 				     (substring data i (min (+ i nbytes) len)))))
 	    (setq i (+ i 1 nbytes))))))))
 
+(make-obsolete 'image-jpeg-p nil "27.1")
+
 
 ;;;###autoload
 (defun image-type-from-data (data)
-- 
2.17.0.775.ge144d126d7

Re: string-to-unibyte in image-jpeg-p

[Stefan Monnier]

> Maybe so, but I prefer Paul's way.  We should be very careful not to
> remove features hastily, without due heads-up to possible users.

We could remove it from master and see: if noone screams before the
release of 27.1 I think it'd be safe to say that it's indeed not used.


        Stefan

Re: string-to-unibyte in image-jpeg-p

[Eli Zaretskii]

> Date: Thu, 24 May 2018 18:19:28 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> > ;; Used to be in image-type-header-regexps, but now not used anywhere
> > ;; (since 2009-08-28).
> > 
> > so it was an internal function in any case, so I think itʼs fair game
> > to just remove it.
> 
> Maybe so, but I prefer Paul's way.  We should be very careful not to
> remove features hastily, without due heads-up to possible users.

And in any case, I think we should un-deprecate string-to-unibyte.

Re: string-to-unibyte in image-jpeg-p

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Thu, 24 May 2018 11:59:47 -0400
> 
> > Maybe so, but I prefer Paul's way.  We should be very careful not to
> > remove features hastily, without due heads-up to possible users.
> 
> We could remove it from master and see: if noone screams before the
> release of 27.1 I think it'd be safe to say that it's indeed not used.

That reminds me of an old joke: if you want to know whether a chip is
in order, plug it into mains: if it explodes, it was in order.

Re: string-to-unibyte in image-jpeg-p

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Thu, 24 May 2018 17:31:40 +0200
> 
> * lisp/image.el (image-jpeg-p): Declare obsolete.
> ---
>  lisp/image.el | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lisp/image.el b/lisp/image.el
> index e084fe329a..95acda0b07 100644
> --- a/lisp/image.el
> +++ b/lisp/image.el
> @@ -270,6 +270,8 @@ image-jpeg-p
>  				     (substring data i (min (+ i nbytes) len)))))
>  	    (setq i (+ i 1 nbytes))))))))
>  
> +(make-obsolete 'image-jpeg-p nil "27.1")

I'd prefer an explicit text as the 2nd arg, something that will warn
about impending removal of the function.

Thanks.

Re: string-to-unibyte in image-jpeg-p

[Stefan Monnier]

>> > Maybe so, but I prefer Paul's way.  We should be very careful not to
>> > remove features hastily, without due heads-up to possible users.
>> We could remove it from master and see: if noone screams before the
>> release of 27.1 I think it'd be safe to say that it's indeed not used.
> That reminds me of an old joke: if you want to know whether a chip is
> in order, plug it into mains: if it explodes, it was in order.

FWIW I'm in favor of applying this principle fairly systematically, e.g.:
functions, vars, and packages declared obsolete in the previous release
should be completely disabled on the master branch (probably with
a mechanism to re-enable them individually, but this mechanism should be
somewhat costly/annoying to the user).  Otherwise it's too easy for package
authors to miss/ignore obsolescence warnings and then be "surprised"
when we finally remove the feature several years later.


        Stefan

Re: string-to-unibyte in image-jpeg-p

[Stefan Monnier]

> +(make-obsolete 'image-jpeg-p nil "27.1")

Nowadays I'd recommend the use of (declare (obsolete nil "27.1"))


        Stefan

Re: string-to-unibyte in image-jpeg-p

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Thu, 24 May 2018 15:51:48 -0400
> 
> > +(make-obsolete 'image-jpeg-p nil "27.1")
> 
> Nowadays I'd recommend the use of (declare (obsolete nil "27.1"))

Why?  What are the factors to consider?

Re: string-to-unibyte in image-jpeg-p

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Thu, 24 May 2018 15:51:06 -0400
> 
> >> > Maybe so, but I prefer Paul's way.  We should be very careful not to
> >> > remove features hastily, without due heads-up to possible users.
> >> We could remove it from master and see: if noone screams before the
> >> release of 27.1 I think it'd be safe to say that it's indeed not used.
> > That reminds me of an old joke: if you want to know whether a chip is
> > in order, plug it into mains: if it explodes, it was in order.
> 
> FWIW I'm in favor of applying this principle fairly systematically

And I'm against it, FWIW.

Re: string-to-unibyte in image-jpeg-p

[Stefan Monnier]

>> > +(make-obsolete 'image-jpeg-p nil "27.1")
>> Nowadays I'd recommend the use of (declare (obsolete nil "27.1"))
> Why?  What are the factors to consider?

My recommendation?


        Stefan

Re: string-to-unibyte in image-jpeg-p

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: emacs-devel@gnu.org
> Date: Fri, 25 May 2018 14:01:00 -0400
> 
> >> > +(make-obsolete 'image-jpeg-p nil "27.1")
> >> Nowadays I'd recommend the use of (declare (obsolete nil "27.1"))
> > Why?  What are the factors to consider?
> 
> My recommendation?

I was hoping to hear some of the reasons why you recommend that, with
the intent of perhaps add some of them to the ELisp manual.

Re: string-to-unibyte in image-jpeg-p

[Stefan Monnier]

>> >> > +(make-obsolete 'image-jpeg-p nil "27.1")
>> >> Nowadays I'd recommend the use of (declare (obsolete nil "27.1"))
>> > Why?  What are the factors to consider?
>> My recommendation?
> I was hoping to hear some of the reasons why you recommend that,

I know, but I must admit that it's largely a question of taste.
I wrote the code that makes it possible, so I'm biased.  Still,
regarding my motivation for writing this code:
- I like the fact that it avoids duplicating the function name.
- It makes it possible for C-M-x to work correctly (e.g. remove the
  obsolescence if you remove the `obsolete` from the `declare`
  (or if you C-M-x on a completely different definition which happens to
  have the same name) tho admittedly the current implementation doesn't
  actually do it).
- More generally, it attaches the declaration to the function's
  *definition* rather than to its *name*, which I think is TRT (even
  tho, again, the underlying implementation still ends up associating
  the two via the name).
- I guess it's more declarative (tho this word tends to be used in
  programming languages a bit like "goodness" in cereal packages or
  the way "GT" was used in cars (back when I was interested in them)).


-- Stefan

Re: string-to-unibyte in image-jpeg-p

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: emacs-devel@gnu.org
> Date: Fri, 25 May 2018 16:39:06 -0400
> 
> - I like the fact that it avoids duplicating the function name.
> - It makes it possible for C-M-x to work correctly (e.g. remove the
>   obsolescence if you remove the `obsolete` from the `declare`
>   (or if you C-M-x on a completely different definition which happens to
>   have the same name) tho admittedly the current implementation doesn't
>   actually do it).
> - More generally, it attaches the declaration to the function's
>   *definition* rather than to its *name*, which I think is TRT (even
>   tho, again, the underlying implementation still ends up associating
>   the two via the name).
> - I guess it's more declarative (tho this word tends to be used in
>   programming languages a bit like "goodness" in cereal packages or
>   the way "GT" was used in cars (back when I was interested in them)).

Thanks, I will think what should be said in the manual based on this.

Re: string-to-unibyte in image-jpeg-p

[Stefan Monnier]

>> > That reminds me of an old joke: if you want to know whether a chip is
>> > in order, plug it into mains: if it explodes, it was in order.
>> FWIW I'm in favor of applying this principle fairly systematically
> And I'm against it, FWIW.

Also FWIW, this opinion of mine is not new, and even while I was
maintainer I didn't dare to act on it.


        Stefan

Re: string-to-unibyte in image-jpeg-p

[Robert Pluim]

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

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

>> +(make-obsolete 'image-jpeg-p nil "27.1")
>
> Nowadays I'd recommend the use of (declare (obsolete nil "27.1"))

That does feel more natural. Updated patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Obsolete-image-jpeg-p.patch --]
[-- Type: text/x-patch, Size: 900 bytes --]

From 048c2ec89a9053ad1dfe0fef3bd4c7154fb5db73 Mon Sep 17 00:00:00 2001
From: Robert Pluim <rpluim@gmail.com>
Date: Thu, 24 May 2018 17:28:11 +0200
Subject: [PATCH] Obsolete image-jpeg-p
To: emacs-devel@gnu.org

* lisp/image.el (image-jpeg-p): Declare obsolete.
---
 lisp/image.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/image.el b/lisp/image.el
index e084fe329a..8d12b680ea 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -248,6 +248,7 @@ image-load-path-for-library
 ;; Used to be in image-type-header-regexps, but now not used anywhere
 ;; (since 2009-08-28).
 (defun image-jpeg-p (data)
+  (declare (obsolete "It is unused inside Emacs and will be removed." "27.1"))
   "Value is non-nil if DATA, a string, consists of JFIF image data.
 We accept the tag Exif because that is the same format."
   (setq data (ignore-errors (string-to-unibyte data)))
-- 
2.17.0.775.ge144d126d7

Re: string-to-unibyte in image-jpeg-p

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Thu, 31 May 2018 12:40:55 +0200
> Cc: emacs-devel@gnu.org
> 
> > Nowadays I'd recommend the use of (declare (obsolete nil "27.1"))
> 
> That does feel more natural. Updated patch.

LGTM, thanks.

Re: string-to-unibyte in image-jpeg-p

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Date: Thu, 31 May 2018 12:40:55 +0200
>> Cc: emacs-devel@gnu.org
>> 
>> > Nowadays I'd recommend the use of (declare (obsolete nil "27.1"))
>> 
>> That does feel more natural. Updated patch.
>
> LGTM, thanks.

Thanks, pushed as c0a0351249

Stefan, did you plan to update the manual description of (declare
(obsolete))?

Robert