bug#51484: [PATCH] Move runtime check for recent giflib to compile time

bug#51484: [PATCH] Move runtime check for recent giflib to compile time

[Stefan Kangas]

Severity: wishlist

I'm looking into some bugs in how we handle gifs, and I see that we
check for GIFLIB_MAJOR at runtime.  Is there any reason not to do it at
compile-time as in the attached patch?

I expect that GCC is smart enough to see that "5 < 4" is always false
and optimize this all away, and probably also won't include unused
static variables, so maybe this doesn't matter.  But I think it's nice
to be a bit more explicit, and I guess it can't hurt to see warnings if
anyone tries using interlace_start and interlace_increment outside of
their intended use.

bug#51484: [PATCH] Move runtime check for recent giflib to compile time

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> I'm looking into some bugs in how we handle gifs, and I see that we
> check for GIFLIB_MAJOR at runtime.  Is there any reason not to do it at
> compile-time as in the attached patch?

-ENOPATCH

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

bug#51484: [PATCH] Move runtime check for recent giflib to compile time

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Fri, 29 Oct 2021 07:42:56 -0700
> 
> I'm looking into some bugs in how we handle gifs, and I see that we
> check for GIFLIB_MAJOR at runtime.

Only once, right?

> Is there any reason not to do it at compile-time as in the attached
> patch?

(What patch?)

The compiler converts that into a run-time constant anyway.  The
reason for using such "run-time" testing is usually to have a more
readable code, since #ifdef's make the code harder to read.  There are
no downsides, since the test is compiled away.

> I expect that GCC is smart enough to see that "5 < 4" is always false
> and optimize this all away, and probably also won't include unused
> static variables, so maybe this doesn't matter.  But I think it's nice
> to be a bit more explicit, and I guess it can't hurt to see warnings if
> anyone tries using interlace_start and interlace_increment outside of
> their intended use.

I don't think I understand what scenario you have in mind where having
an #ifdef would be better.  Please elaborate.

bug#51484: [PATCH] Move runtime check for recent giflib to compile time

[Stefan Kangas]

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

Eli Zaretskii <eliz@gnu.org> writes:

> (What patch?)

Oops, let's try that again.

> The compiler converts that into a run-time constant anyway.  The
> reason for using such "run-time" testing is usually to have a more
> readable code, since #ifdef's make the code harder to read.  There are
> no downsides, since the test is compiled away.

Right, hence my asking what you all think of the patch.  Which is now
hopefully also attached.

[-- Attachment #2: 0001-Move-runtime-check-for-recent-giflib-to-compile-time.patch --]
[-- Type: text/x-diff, Size: 1656 bytes --]

From ee975ee6b2c9496c49cfb36aa22acab9ceca2d32 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Fri, 29 Oct 2021 15:24:26 +0200
Subject: [PATCH] Move runtime check for recent giflib to compile time

* src/image.c
(interlace_start, interlace_increment) [GIFLIB_MAJOR < 5]:
Don't define when using recent giflib.
(gif_load): Replace runtime check for "GIFLIB_MAJOR < 5" with
preprocessor directive.
---
 src/image.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/image.c b/src/image.c
index 99533bbd1b..0f105131aa 100644
--- a/src/image.c
+++ b/src/image.c
@@ -8358,8 +8358,10 @@ gif_close (GifFileType *gif, int *err)
 /* Load GIF image IMG for use on frame F.  Value is true if
    successful.  */
 
+#if GIFLIB_MAJOR < 5
 static const int interlace_start[] = {0, 4, 2, 1};
 static const int interlace_increment[] = {8, 8, 4, 2};
+#endif
 
 #define GIF_LOCAL_DESCRIPTOR_EXTENSION 249
 
@@ -8626,7 +8628,8 @@ gif_load (struct frame *f, struct image *img)
 	  }
 
       /* Apply the pixel values.  */
-      if (GIFLIB_MAJOR < 5 && gif->SavedImages[j].ImageDesc.Interlace)
+#if GIFLIB_MAJOR < 5
+      if (gif->SavedImages[j].ImageDesc.Interlace)
 	{
 	  int row, pass;
 
@@ -8650,6 +8653,7 @@ gif_load (struct frame *f, struct image *img)
 	}
       else
 	{
+#endif
           for (y = 0; y < subimg_height; ++y)
 	    for (x = 0; x < subimg_width; ++x)
 	      {
@@ -8661,7 +8665,9 @@ gif_load (struct frame *f, struct image *img)
                   }
 	      }
 	}
+#if GIFLIB_MAJOR < 5
     }
+#endif
 
 #ifdef COLOR_TABLE_SUPPORT
   img->colors = colors_in_color_table (&img->ncolors);
-- 
2.30.2

bug#51484: [PATCH] Move runtime check for recent giflib to compile time

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Fri, 29 Oct 2021 09:30:40 -0700
> Cc: 51484@debbugs.gnu.org
> 
> > The compiler converts that into a run-time constant anyway.  The
> > reason for using such "run-time" testing is usually to have a more
> > readable code, since #ifdef's make the code harder to read.  There are
> > no downsides, since the test is compiled away.
> 
> Right, hence my asking what you all think of the patch.  Which is now
> hopefully also attached.

I generally prefer to avoid #ifdef's, so I think the change you
propose is not for the better.  We have similar code elsewhere in
Emacs, and there's nothing wrong with it.

bug#51484: [PATCH] Move runtime check for recent giflib to compile time

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> I generally prefer to avoid #ifdef's, so I think the change you
> propose is not for the better.  We have similar code elsewhere in
> Emacs, and there's nothing wrong with it.

OK, thanks for taking a look.  I appreciate you taking the time to
explain your thinking as well, it is most helpful.

I'm closing this bug report.