bug#36403: 27.0.50; Trivial image.c bugs

["Basil L. Contovounesios" via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

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

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

> Pip Cet <pipcet@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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Touch-up-image-circular-tests.el.patch --]
[-- Type: text/x-diff, Size: 3688 bytes --]

From d22733866d5343b7a5a38ad8dc76312b1b0ed1f5 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Mon, 3 Oct 2022 23:34:07 +0300
Subject: [PATCH] Touch up image-circular-tests.el

* test/manual/image-circular-tests.el
(image-test-duplicate-keywords, image-test-circular-plist)
(image-test-:type-property-value, image-test-circular-specs): Skip
tests when there is no image support.  Avoid wrapping entire test
bodies in should or should-error; wrap only the relevant forms
within the test body.  Simplify with printed notation in place of
function calls where applicable.  Wrap long docstrings.  (Bug#36403)
---
 test/manual/image-circular-tests.el | 38 ++++++++++++++++-------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/test/manual/image-circular-tests.el b/test/manual/image-circular-tests.el
index 1299970f82..df9031664b 100644
--- a/test/manual/image-circular-tests.el
+++ b/test/manual/image-circular-tests.el
@@ -29,6 +29,7 @@
 
 (ert-deftest image-test-duplicate-keywords ()
   "Test that duplicate keywords in an image spec lead to rejection."
+  (skip-unless (display-images-p))
   (should-error (image-size `(image :type xbm :type xbm
                                     :data-width 1 :data-height 1
                                     :data ,(bool-vector t))
@@ -36,33 +37,36 @@ image-test-duplicate-keywords
 
 (ert-deftest image-test-circular-plist ()
   "Test that a circular image spec is rejected."
-  (should-error
-   (let ((l `(image :type xbm :data-width 1 :data-height 1
-                    :data ,(bool-vector t))))
-     (setcdr (last l) '#1=(:invalid . #1#))
-     (image-size l t))))
+  (skip-unless (display-images-p))
+  (let ((spec `(image :type xbm :data-width 1 :data-height 1
+                      :data ,(bool-vector t)
+                      . ,'#1=(:invalid . #1#))))
+    (should-error (image-size spec t))))
 
 (ert-deftest image-test-:type-property-value ()
   "Test that :type is allowed as a property value in an image spec."
+  (skip-unless (display-images-p))
   (should (equal (image-size `(image :dummy :type :type xbm
                                      :data-width 1 :data-height 1
                                      :data ,(bool-vector t))
                              t)
-                 (cons 1 1))))
+                 '(1 . 1))))
 
 (ert-deftest image-test-circular-specs ()
-  "Test that circular image spec property values do not cause infinite recursion."
-  (should
-   (let* ((circ1 (cons :dummy nil))
-          (circ2 (cons :dummy nil))
-          (spec1 `(image :type xbm :data-width 1 :data-height 1
-                         :data ,(bool-vector 1) :ignored ,circ1))
-          (spec2 `(image :type xbm :data-width 1 :data-height 1
+  "Test with circular image spec property values.
+In particular, test that they do not cause infinite recursion."
+  (skip-unless (display-images-p))
+  ;; Two copies needed to warm up image cache.
+  (let* ((circ1 (list :dummy))
+         (circ2 (list :dummy))
+         (spec1 `(image :type xbm :data-width 1 :data-height 1
+                        :data ,(bool-vector 1) :ignored ,circ1))
+         (spec2 `(image :type xbm :data-width 1 :data-height 1
                         :data ,(bool-vector 1) :ignored ,circ2)))
-     (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)))))
 
 (provide 'image-circular-tests)
 ;;; image-circular-tests.el ends here.
-- 
2.35.1