GNU bug report logs - #36403
27.0.50; Trivial image.c bugs

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 36403 in the body.
You can then email your comments to 36403 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.50; Trivial image.c bugs
Date: Thu, 27 Jun 2019 16:28:05 +0000

[Message part 1 (text/plain, inline)]

These are all in the category "Lisp code does something silly, the
image code breaks".

(let ((l `(image :type xbm :type xbm :height 1 :width 1 :data
,(bool-vector t))))
  (insert-image l))

inserts an image. It should consider the spec erroneous.
--
(let ((tail (cons :invalid nil)))
   (setcdr tail tail)
   (insert-image `(image :type xbm . ,tail)))

causes an infinite loop. It should be considered invalid.
--
(insert-image `(image :dummy :type :type xbm :height 1 :width 1 :data
,(bool-vector t)))

produces an error. It should arguably behave the same as

(insert-image `(image :dummy :dummy :type xbm :height 1 :width 1 :data
,(bool-vector t)))
--
(let* ((circ1 (cons :dummy nil))
       (circ2 (cons :dummy nil))
       (spec1 `(image :type xbm :width 1 :height 1 :data ,(bool-vector
1) :ignored ,circ1))
       (spec2 `(image :type xbm :width 1 :height 1 :data ,(bool-vector
1) :ignored ,circ2)))
  (setcdr circ1 circ1)
  (setcdr circ2 circ2)
  (insert-image spec1)
  (insert-image spec2))

livelocks emacs somehow. It should...I don't know. Abort because the
spec is circular? Not compare specs using Fequal?
--
(insert-image `(image :type postscript :pt-width 100 :pt-height 100
              :ascent 0
              :bounding-box (0 0 100 100) :file "dummy.ps"
              :loader ,(lambda (frame spec width height id colors)
                 (setf (plist-get spec :ascent)
                       -1))))

livelocks Emacs in the display code. It should automatically switch to
the buffer called "image.c" and rewrite the code there not to call
Lisp.
--
These probably aren't worth fixing in their own right, but someone
might think image.c is a good place to take plist handling code
from...

I think with the exception of the contrived last example, these are
all easy to fix, but a bit harder to fix well. I've tried to do the
former, for now, but I'd welcome any help for me to do the latter.

[0001-Fix-minor-image-bugs.patch (text/x-patch, attachment)]

Message #8 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: 36403 <at> debbugs.gnu.org
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Thu, 27 Jun 2019 20:40:07 +0300

> From: Pip Cet <pipcet <at> gmail.com>
> Date: Thu, 27 Jun 2019 16:28:05 +0000
> 
> I think with the exception of the contrived last example, these are
> all easy to fix, but a bit harder to fix well. I've tried to do the
> former, for now, but I'd welcome any help for me to do the latter.

Thanks, but please also add tests which verify that these problems no
longer happen after the fix.

Message #11 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36403 <at> debbugs.gnu.org
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Fri, 28 Jun 2019 15:05:30 +0000

[Message part 1 (text/plain, inline)]

On Thu, Jun 27, 2019 at 5:40 PM Eli Zaretskii <eliz <at> gnu.org> wrote:
> > From: Pip Cet <pipcet <at> gmail.com>
> > Date: Thu, 27 Jun 2019 16:28:05 +0000
> >
> > I think with the exception of the contrived last example, these are
> > all easy to fix, but a bit harder to fix well. I've tried to do the
> > former, for now, but I'd welcome any help for me to do the latter.
>
> Thanks, but please also add tests which verify that these problems no
> longer happen after the fix.

Thanks for reminding me of that! I looked into that, and it seems
there are no image tests so far.

Attached patch has tests and fixes.

[0001-Fix-minor-bugs-in-image.c.patch (text/x-patch, attachment)]

Message #14 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: 36403 <at> debbugs.gnu.org
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Fri, 28 Jun 2019 22:52:50 +0300

> From: Pip Cet <pipcet <at> gmail.com>
> Date: Fri, 28 Jun 2019 15:05:30 +0000
> Cc: 36403 <at> debbugs.gnu.org
> 
> > Thanks, but please also add tests which verify that these problems no
> > longer happen after the fix.
> 
> Thanks for reminding me of that! I looked into that, and it seems
> there are no image tests so far.

There are some in test/manual.  they are there because they need to be
run interactively.

> Attached patch has tests and fixes.

Thanks.  Let's wait for a few days to let others comment.

Message #17 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36403 <at> debbugs.gnu.org
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Mon, 22 Jul 2019 02:55:50 +0000

[Message part 1 (text/plain, inline)]

On Fri, Jun 28, 2019 at 7:53 PM Eli Zaretskii <eliz <at> gnu.org> wrote:
> > Attached patch has tests and fixes.
>
> Thanks.  Let's wait for a few days to let others comment.

Rebased patch attached.

[0001-Fix-minor-bugs-in-image.c-bug-36403.patch (text/x-patch, attachment)]

Message #20 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: 36403 <at> debbugs.gnu.org
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Fri, 26 Jul 2019 09:56:46 +0300

> From: Pip Cet <pipcet <at> gmail.com>
> Date: Mon, 22 Jul 2019 02:55:50 +0000
> Cc: 36403 <at> debbugs.gnu.org
> 
> On Fri, Jun 28, 2019 at 7:53 PM Eli Zaretskii <eliz <at> gnu.org> wrote:
> > > Attached patch has tests and fixes.
> >
> > Thanks.  Let's wait for a few days to let others comment.
> 
> Rebased patch attached.

Thanks.

Now that I applied this and looked into the results and the code, I
have a few questions/comments.  Sorry I didn't see this earlier.

In your bug report, you say, among other things:

> (insert-image `(image :dummy :type :type xbm :height 1 :width 1 :data
> ,(bool-vector t)))
> 
> produces an error. It should arguably behave the same as
> 
> (insert-image `(image :dummy :dummy :type xbm :height 1 :width 1 :data
> ,(bool-vector t)))

Can you explain why these two are equivalent?

> (equal_lists): Introduce.
> (search_image_cache): Use `equal_lists'.

I don't think I understand why we need this new function.  Can you explain?

Finally, 2 of the tests fail for me:

  $ make src/image-tests.log
    ELC      src/image-tests.elc
    GEN      src/image-tests.log
  Running 4 tests (2019-07-26 09:49:33+0300, selector `(not (tag :unstable))')
  Test image-test-:type-property-value backtrace:
    signal(error ("Window system frame should be used"))
    apply(signal (error ("Window system frame should be used")))
    (setq value-13 (apply fn-11 args-12))
    (unwind-protect (setq value-13 (apply fn-11 args-12)) (setq form-des
    (if (unwind-protect (setq value-13 (apply fn-11 args-12)) (setq form
    (let (form-description-15) (if (unwind-protect (setq value-13 (apply
    (let ((value-13 'ert-form-evaluation-aborted-14)) (let (form-descrip
    (let* ((fn-11 #'equal) (args-12 (condition-case err (let ((signal-ho
    (lambda nil (let* ((fn-11 #'equal) (args-12 (condition-case err (let
    ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
    ert-run-test(#s(ert-test :name image-test-:type-property-value :docu
    ert-run-or-rerun-test(#s(ert--stats :selector (not (tag :unstable))
    ert-run-tests((not (tag :unstable)) #f(compiled-function (event-type
    ert-run-tests-batch((not (tag :unstable)))
    ert-run-tests-batch-and-exit((not (tag :unstable)))
    eval((ert-run-tests-batch-and-exit '(not (tag :unstable))) t)
    command-line-1((#("-L" 0 2 (charset cp862)) #(";." 0 2 (charset cp86
    command-line()
    normal-top-level()
  Test image-test-:type-property-value condition:
      (error "Window system frame should be used")
     FAILED  1/4  image-test-:type-property-value (0.000000 sec)
     passed  2/4  image-test-circular-plist (0.000000 sec)
  Test image-test-circular-specs backtrace:
    image-size((image :type xbm :width 1 :height 1 :data #&1"\1" :ignore
    (equal (image-size spec1 t) (cons 1 1))
    (and (equal (image-size spec1 t) (cons 1 1)) (equal (image-size spec
    (let* ((circ1 (cons :dummy nil)) (circ2 (cons :dummy nil)) (spec1 (l
    (setq value-16 (let* ((circ1 (cons :dummy nil)) (circ2 (cons :dummy
    (unwind-protect (setq value-16 (let* ((circ1 (cons :dummy nil)) (cir
    (if (unwind-protect (setq value-16 (let* ((circ1 (cons :dummy nil))
    (let (form-description-17) (if (unwind-protect (setq value-16 (let*
    (let ((value-16 (gensym "ert-form-evaluation-aborted-"))) (let (form
    (lambda nil (let ((value-16 (gensym "ert-form-evaluation-aborted-"))
    ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
    ert-run-test(#s(ert-test :name image-test-circular-specs :documentat
    ert-run-or-rerun-test(#s(ert--stats :selector (not (tag :unstable))
    ert-run-tests((not (tag :unstable)) #f(compiled-function (event-type
    ert-run-tests-batch((not (tag :unstable)))
    ert-run-tests-batch-and-exit((not (tag :unstable)))
    eval((ert-run-tests-batch-and-exit '(not (tag :unstable))) t)
    command-line-1((#("-L" 0 2 (charset cp862)) #(";." 0 2 (charset cp86
    command-line()
    normal-top-level()
  Test image-test-circular-specs condition:
      (error "Window system frame should be used")
     FAILED  3/4  image-test-circular-specs (0.000000 sec)
     passed  4/4  image-test-duplicate-keywords (0.000000 sec)

  Ran 4 tests, 2 results as expected, 2 unexpected (2019-07-26 09:49:35+0300, 1.906250 sec)

  2 unexpected results:
     FAILED  image-test-:type-property-value
     FAILED  image-test-circular-specs

Message #23 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36403 <at> debbugs.gnu.org
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Sun, 28 Jul 2019 14:50:47 +0000

On Fri, Jul 26, 2019 at 6:56 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
> > From: Pip Cet <pipcet <at> gmail.com>
> > Date: Mon, 22 Jul 2019 02:55:50 +0000
> > Cc: 36403 <at> debbugs.gnu.org
> >
> > On Fri, Jun 28, 2019 at 7:53 PM Eli Zaretskii <eliz <at> gnu.org> wrote:
> > > > Attached patch has tests and fixes.
> > >
> > > Thanks.  Let's wait for a few days to let others comment.
> >
> > Rebased patch attached.
>
> Thanks.
>
> Now that I applied this and looked into the results and the code, I
> have a few questions/comments.  Sorry I didn't see this earlier.

No problem at all, and thank you, as always, for your thoughtful comments!

> In your bug report, you say, among other things:
>
> > (insert-image `(image :dummy :type :type xbm :height 1 :width 1 :data
> > ,(bool-vector t)))
> >
> > produces an error. It should arguably behave the same as
> >
> > (insert-image `(image :dummy :dummy :type xbm :height 1 :width 1 :data
> > ,(bool-vector t)))
>
> Can you explain why these two are equivalent?

The ":dummy" property should be ignored, whether its value is ":dummy"
or ":type"; previously, we used the first occurence of :type even if
it was at an odd offset in the plist.

> > (equal_lists): Introduce.
> > (search_image_cache): Use `equal_lists'.
>
> I don't think I understand why we need this new function.  Can you explain?

IIRC, Fequal throwing a signal at this point caused a livelock, so we
needed a stricter check. I'll look into it again to see whether
there's a better alternative.

Thanks again!

Message #28 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 36403 <at> debbugs.gnu.org
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Tue, 24 Sep 2019 18:26:55 +0200

Pip Cet <pipcet <at> gmail.com> writes:

> IIRC, Fequal throwing a signal at this point caused a livelock, so we
> needed a stricter check. I'll look into it again to see whether
> there's a better alternative.

This was eight weeks ago, and as far as I can tell, the patch (which
looks sensible) wasn't applied.  Has there been any further progress
here?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #31 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 36403 <at> debbugs.gnu.org
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Mon, 03 Aug 2020 09:47:18 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Pip Cet <pipcet <at> gmail.com> writes:
>
>> IIRC, Fequal throwing a signal at this point caused a livelock, so we
>> needed a stricter check. I'll look into it again to see whether
>> there's a better alternative.
>
> This was eight weeks ago, and as far as I can tell, the patch (which
> looks sensible) wasn't applied.  Has there been any further progress
> here?

And that was 44 weeks ago.  :-)  The patch still applies.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #34 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 36403 <at> debbugs.gnu.org
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Tue, 18 Aug 2020 18:28:19 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Lars Ingebrigtsen <larsi <at> gnus.org> writes:
>
>> Pip Cet <pipcet <at> gmail.com> writes:
>>
>>> IIRC, Fequal throwing a signal at this point caused a livelock, so we
>>> needed a stricter check. I'll look into it again to see whether
>>> there's a better alternative.
>>
>> This was eight weeks ago, and as far as I can tell, the patch (which
>> looks sensible) wasn't applied.  Has there been any further progress
>> here?
>
> And that was 44 weeks ago.  :-)  The patch still applies.

I've now applied the patch, but I moved the test suite to test/manual --
they call `image-size', which requires a frame to be present...  which
isn't the case when running "make test", unfortunately.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #41 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: Alan Third <alan <at> idiocy.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 36403 <at> debbugs.gnu.org, Pip Cet <pipcet <at> gmail.com>
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Fri, 21 Aug 2020 01:03:34 +0200 (CEST)

On Tue, Aug 18, 2020 at 06:28:19PM +0200, Lars Ingebrigtsen wrote:
> Lars Ingebrigtsen <larsi <at> gnus.org> writes:
> 
> > Lars Ingebrigtsen <larsi <at> gnus.org> writes:
> >
> >> Pip Cet <pipcet <at> gmail.com> writes:
> >>
> >>> IIRC, Fequal throwing a signal at this point caused a livelock, so we
> >>> needed a stricter check. I'll look into it again to see whether
> >>> there's a better alternative.
> >>
> >> This was eight weeks ago, and as far as I can tell, the patch (which
> >> looks sensible) wasn't applied.  Has there been any further progress
> >> here?
> >
> > And that was 44 weeks ago.  :-)  The patch still applies.
> 
> I've now applied the patch, but I moved the test suite to test/manual --
> they call `image-size', which requires a frame to be present...  which
> isn't the case when running "make test", unfortunately.

Hi, this patch appears to have broken something (in NS at least) and
now interactively resizing an image no longer causes the image to be
immediately redisplayed. I have to force a redisplay some other way
before I see the resized image.
-- 
Alan Third

Message #44 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Alan Third <alan <at> idiocy.org>
Cc: 36403 <at> debbugs.gnu.org, Pip Cet <pipcet <at> gmail.com>
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Fri, 21 Aug 2020 01:13:58 +0200

Alan Third <alan <at> idiocy.org> writes:

> Hi, this patch appears to have broken something (in NS at least) and
> now interactively resizing an image no longer causes the image to be
> immediately redisplayed. I have to force a redisplay some other way
> before I see the resized image.

Yup.  This patch fixes things for me (on Debian)...

diff --git a/src/image.c b/src/image.c
index 643b3d0a1f..ceb690ed0a 100644
--- a/src/image.c
+++ b/src/image.c
@@ -1633,7 +1633,7 @@ search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash)
 
   for (img = c->buckets[i]; img; img = img->next)
     if (img->hash == hash
-	&& !equal_lists (img->spec, spec)
+	&& !NILP (Fequal (img->spec, spec))
 	&& img->frame_foreground == FRAME_FOREGROUND_PIXEL (f)
 	&& img->frame_background == FRAME_BACKGROUND_PIXEL (f))
       break;

The equal_lists thing looks sensible, but I guess we do destructive
alterations with +/-, possibly, to equal is the correct thing, not a
list-of-eqs?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #47 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Alan Third <alan <at> idiocy.org>
Cc: 36403 <at> debbugs.gnu.org, Pip Cet <pipcet <at> gmail.com>
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Fri, 21 Aug 2020 01:17:28 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> The equal_lists thing looks sensible, but I guess we do destructive
> alterations with +/-, possibly, to equal is the correct thing, not a
> list-of-eqs?

That should be "so equal is the correct thing"...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #50 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Alan Third <alan <at> idiocy.org>
Cc: 36403 <at> debbugs.gnu.org, Pip Cet <pipcet <at> gmail.com>
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Fri, 21 Aug 2020 01:32:27 +0200

D'oh.  Pip's patch here just had reverse logic -- removing the ! from
before the equal_lists fixes the issue for me, so I've now pushed that
fix.

  for (img = c->buckets[i]; img; img = img->next)
     if (img->hash == hash
-	&& !NILP (Fequal (img->spec, spec))
+	&& !equal_lists (img->spec, spec)
 	&& img->frame_foreground == FRAME_FOREGROUND_PIXEL (f)
 	&& img->frame_background == FRAME_BACKGROUND_PIXEL (f))

I guess the !NILP (Fequal...) idiom tainted the rewrite...

I've often wondered why we use !NILP instead of, like TRUEP or
something.  !NILP doesn't feel very natural.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #53 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Alan Third <alan <at> idiocy.org>, 36403 <at> debbugs.gnu.org
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Fri, 21 Aug 2020 09:26:10 +0000

On Thu, Aug 20, 2020 at 11:32 PM Lars Ingebrigtsen <larsi <at> gnus.org> wrote:
>   for (img = c->buckets[i]; img; img = img->next)
>      if (img->hash == hash
> -       && !NILP (Fequal (img->spec, spec))
> +       && !equal_lists (img->spec, spec)
>         && img->frame_foreground == FRAME_FOREGROUND_PIXEL (f)
>         && img->frame_background == FRAME_BACKGROUND_PIXEL (f))
>
> I guess the !NILP (Fequal...) idiom tainted the rewrite...

I can confirm it did. However, my stupidity is not a very good
argument for changing Emacs, or all of it would need changing :-)

> I've often wondered why we use !NILP instead of, like TRUEP or
> something.  !NILP doesn't feel very natural.

Paul's suggestion was to use equal () instead of !NILP (Fequal (...)).
I'm against that, because the F in Fequal kind of hints at the
difficulties of using equal, of which there are many: in the current
implementation, it can signal, quit, be asymmetric (signalling for
(equal a b) whereas (equal b a) works), and is susceptible to equality
bombs that take forever to compare.  It cannot call Lisp, yet, but I
wouldn't be surprised if that changes. It's really not a function you
should use from C very often, and using it as a hash table predicate
is often the wrong thing to do.

Replacing !NILP is a better idea, but I'm struggling to come up with a
good name for that. But even a bad name would be an improvement.

Message #56 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: Alan Third <alan <at> idiocy.org>, 36403 <at> debbugs.gnu.org
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Fri, 21 Aug 2020 13:26:25 +0200

Pip Cet <pipcet <at> gmail.com> writes:

> Paul's suggestion was to use equal () instead of !NILP (Fequal (...)).
> I'm against that, because the F in Fequal kind of hints at the
> difficulties of using equal, of which there are many: in the current
> implementation, it can signal, quit, be asymmetric (signalling for
> (equal a b) whereas (equal b a) works), and is susceptible to equality
> bombs that take forever to compare.

Yeah, your equal_lists is better in all ways, I think.  It should be
much faster, too -- Fequal on a list checks whether the string members
are equal, too, which is slow.  So I think this will speed things up if
you have a buffer that displays images where the data comes from a
string (which can be huge) instead of a file.

> Replacing !NILP is a better idea, but I'm struggling to come up with a
> good name for that. But even a bad name would be an improvement.

TRUEP is kinda obvious, isn't it?  Although I guess some people would
object on the grounds that only t is true, while all other non-nil
values are only trueish.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #63 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Alan Third <alan <at> idiocy.org>, 36403 <at> debbugs.gnu.org,
 Pip Cet <pipcet <at> gmail.com>
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Tue, 04 Oct 2022 16:52:59 +0300

[Message part 1 (text/plain, inline)]

Lars Ingebrigtsen [2020-08-21 13:26 +0200] wrote:

> Pip Cet <pipcet <at> gmail.com> writes:
>
>> Paul's suggestion was to use equal () instead of !NILP (Fequal (...)).
>> I'm against that, because the F in Fequal kind of hints at the
>> difficulties of using equal, of which there are many: in the current
>> implementation, it can signal, quit, be asymmetric (signalling for
>> (equal a b) whereas (equal b a) works), and is susceptible to equality
>> bombs that take forever to compare.
>
> Yeah, your equal_lists is better in all ways, I think.  It should be
> much faster, too -- Fequal on a list checks whether the string members
> are equal, too, which is slow.  So I think this will speed things up if
> you have a buffer that displays images where the data comes from a
> string (which can be huge) instead of a file.

We now have Fequal again:

  Restore Emacs 27 image cache semantics
  ac341cd629 2020-12-09 00:42:11 +0100
  https://git.sv.gnu.org/cgit/emacs.git/commit/?id=ac341cd629

Which means image-test-circular-specs signals:

  (circular-list (:dummy . #0))

So how important is it to support image specs with circular property
values?  Should the test be marked :expected-result :failed?

Either way, WDYT of the attached minor cleanup?

Thanks,

-- 
Basil

[0001-Touch-up-image-circular-tests.el.patch (text/x-diff, attachment)]

Message #66 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: Alan Third <alan <at> idiocy.org>, 36403 <at> debbugs.gnu.org,
 Pip Cet <pipcet <at> gmail.com>
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Tue, 04 Oct 2022 16:06:28 +0200

"Basil L. Contovounesios" <contovob <at> tcd.ie> writes:

> Which means image-test-circular-specs signals:
>
>   (circular-list (:dummy . #0))
>
> So how important is it to support image specs with circular property
> values?  Should the test be marked :expected-result :failed?

Hm...  don't know.

> -     (setcdr circ1 circ1)
> -     (setcdr circ2 circ2)
> -     (and (equal (image-size spec1 t) (cons 1 1))
> -          (equal (image-size spec2 t) (cons 1 1))))))
> +    (setcdr circ1 circ1)
> +    (setcdr circ2 circ2)
> +    (should (equal (image-size spec1 t) '(1 . 1)))
> +    (should (equal (image-size spec2 t) '(1 . 1)))))

(cons 1 1) is not eq to (cons 1 1), but '(1 . 1) may or may not be eq to
'(1 . 1) (depending on whether compiled or phase of the moon), which may
be why that code is written that way?  But in this case, I can't see how
it would make a difference...

Message #69 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Alan Third <alan <at> idiocy.org>, 36403 <at> debbugs.gnu.org,
 Pip Cet <pipcet <at> gmail.com>
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Tue, 04 Oct 2022 21:05:17 +0300

Lars Ingebrigtsen [2022-10-04 16:06 +0200] wrote:

> "Basil L. Contovounesios" <contovob <at> tcd.ie> writes:
>
>> -     (setcdr circ1 circ1)
>> -     (setcdr circ2 circ2)
>> -     (and (equal (image-size spec1 t) (cons 1 1))
>> -          (equal (image-size spec2 t) (cons 1 1))))))
>> +    (setcdr circ1 circ1)
>> +    (setcdr circ2 circ2)
>> +    (should (equal (image-size spec1 t) '(1 . 1)))
>> +    (should (equal (image-size spec2 t) '(1 . 1)))))
>
> (cons 1 1) is not eq to (cons 1 1), but '(1 . 1) may or may not be eq to
> '(1 . 1) (depending on whether compiled or phase of the moon), which may
> be why that code is written that way?  But in this case, I can't see how
> it would make a difference...

It doesn't matter whether the calls to image-size return the same
object.  What matters is that both calls succeed (that they return the
correct result is also nice I guess, if that's what you're into ;).

-- 
Basil

Message #72 received at 36403 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Alan Third <alan <at> idiocy.org>, 36403 <at> debbugs.gnu.org,
 Pip Cet <pipcet <at> gmail.com>
Subject: Re: bug#36403: 27.0.50; Trivial image.c bugs
Date: Sat, 15 Oct 2022 01:14:11 +0300

Lars Ingebrigtsen [2022-10-04 16:06 +0200] wrote:

> "Basil L. Contovounesios" <contovob <at> tcd.ie> writes:
>
>> Which means image-test-circular-specs signals:
>>
>>   (circular-list (:dummy . #0))
>>
>> So how important is it to support image specs with circular property
>> values?  Should the test be marked :expected-result :failed?
>
> Hm...  don't know.

I've marked it as :expected-result :failed for now:

Update image-circular-tests.el
f5c6e628ed 2022-10-15 01:10:31 +0300
https://git.sv.gnu.org/cgit/emacs.git/commit/?id=f5c6e628ed

Thanks,

-- 
Basil

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.