GNU bug report logs - #51484
[PATCH] Move runtime check for recent giflib to compile time

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 51484 in the body.
You can then email your comments to 51484 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: Stefan Kangas <stefan <at> marxist.se>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Move runtime check for recent giflib to compile time
Date: Fri, 29 Oct 2021 07:42:56 -0700

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.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 51484 <at> debbugs.gnu.org
Subject: Re: bug#51484: [PATCH] Move runtime check for recent giflib to
 compile time
Date: Fri, 29 Oct 2021 18:01:54 +0200

Stefan Kangas <stefan <at> 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

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 51484 <at> debbugs.gnu.org
Subject: Re: bug#51484: [PATCH] Move runtime check for recent giflib to compile
 time
Date: Fri, 29 Oct 2021 19:10:30 +0300

> From: Stefan Kangas <stefan <at> 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.

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

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 51484 <at> debbugs.gnu.org
Subject: Re: bug#51484: [PATCH] Move runtime check for recent giflib to
 compile time
Date: Fri, 29 Oct 2021 09:30:40 -0700

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

Eli Zaretskii <eliz <at> 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.

[0001-Move-runtime-check-for-recent-giflib-to-compile-time.patch (text/x-diff, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 51484 <at> debbugs.gnu.org
Subject: Re: bug#51484: [PATCH] Move runtime check for recent giflib to
 compile time
Date: Fri, 29 Oct 2021 20:57:52 +0300

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Fri, 29 Oct 2021 09:30:40 -0700
> Cc: 51484 <at> 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.

Message #22 received at 51484-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 51484-done <at> debbugs.gnu.org
Subject: Re: bug#51484: [PATCH] Move runtime check for recent giflib to
 compile time
Date: Fri, 29 Oct 2021 11:15:04 -0700

Eli Zaretskii <eliz <at> 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.

Previous Next

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