* 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).