* bug#51484: [PATCH] Move runtime check for recent giflib to compile time
@ 2021-10-29 14:42 Stefan Kangas
2021-10-29 16:01 ` Lars Ingebrigtsen
2021-10-29 16:10 ` Eli Zaretskii
0 siblings, 2 replies; 6+ messages in thread
From: Stefan Kangas @ 2021-10-29 14:42 UTC (permalink / raw)
To: 51484
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#51484: [PATCH] Move runtime check for recent giflib to compile time
2021-10-29 14:42 bug#51484: [PATCH] Move runtime check for recent giflib to compile time Stefan Kangas
@ 2021-10-29 16:01 ` Lars Ingebrigtsen
2021-10-29 16:10 ` Eli Zaretskii
1 sibling, 0 replies; 6+ messages in thread
From: Lars Ingebrigtsen @ 2021-10-29 16:01 UTC (permalink / raw)
To: Stefan Kangas; +Cc: 51484
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#51484: [PATCH] Move runtime check for recent giflib to compile time
2021-10-29 14:42 bug#51484: [PATCH] Move runtime check for recent giflib to compile time Stefan Kangas
2021-10-29 16:01 ` Lars Ingebrigtsen
@ 2021-10-29 16:10 ` Eli Zaretskii
2021-10-29 16:30 ` Stefan Kangas
1 sibling, 1 reply; 6+ messages in thread
From: Eli Zaretskii @ 2021-10-29 16:10 UTC (permalink / raw)
To: Stefan Kangas; +Cc: 51484
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#51484: [PATCH] Move runtime check for recent giflib to compile time
2021-10-29 16:10 ` Eli Zaretskii
@ 2021-10-29 16:30 ` Stefan Kangas
2021-10-29 17:57 ` Eli Zaretskii
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Kangas @ 2021-10-29 16:30 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 51484
[-- 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#51484: [PATCH] Move runtime check for recent giflib to compile time
2021-10-29 16:30 ` Stefan Kangas
@ 2021-10-29 17:57 ` Eli Zaretskii
2021-10-29 18:15 ` Stefan Kangas
0 siblings, 1 reply; 6+ messages in thread
From: Eli Zaretskii @ 2021-10-29 17:57 UTC (permalink / raw)
To: Stefan Kangas; +Cc: 51484
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#51484: [PATCH] Move runtime check for recent giflib to compile time
2021-10-29 17:57 ` Eli Zaretskii
@ 2021-10-29 18:15 ` Stefan Kangas
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Kangas @ 2021-10-29 18:15 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 51484-done
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-29 18:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-29 14:42 bug#51484: [PATCH] Move runtime check for recent giflib to compile time Stefan Kangas
2021-10-29 16:01 ` Lars Ingebrigtsen
2021-10-29 16:10 ` Eli Zaretskii
2021-10-29 16:30 ` Stefan Kangas
2021-10-29 17:57 ` Eli Zaretskii
2021-10-29 18:15 ` Stefan Kangas
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).