Fix HAVE_LIBGD usage

[petk]

When PHP gd extension uses the external system GD library, the HAVE_LIBGD preprocessor macro gets defined in Autotools. On Windows it was previously always defined when bundled library is used. This fixes the usage and adds help texts.

[petk]

Probably this symbol can be even removed in later versions since it was originally probably meant to be used differently.

[cmb69]

It seemed to me that HAVE_LIBGD is supposed to mean that there is any libgd available (either system or bundled), and that HAVE_GD_BUNDLED just clarifies if it's the system or the bundled libgd. But seeing

php-src/ext/gd/php_gd.h

Line 24 in d6a75e1

#if defined(HAVE_LIBGD) || defined(HAVE_GD_BUNDLED)

shows that this apparently has been a misunderstanding. So removing the AC_DEFINE(HAVE_LIBGD) from config.w32 makes sense.

Regarding the description: I'm not sure whether the more verbose text is an improvement. The (likely) few users who edit config.h manually need to know what they are doing anyway (otherwise the build may fail, or maybe worse things may happen). On the other hand, I have no strong opinion on this, so being more verbose might be a good idea, but it seems to me that we want to update all relevant descriptions for ext/gd in one go, to avoid users having to rebase multiple times when working on config.(m4|w32).

[petk]

Regarding the description: I'm not sure whether the more verbose text is an improvement. The (likely) few users who edit config.h manually need to know what they are doing anyway...

I'm glad you've brought this up. I guess I'll have to write docs about this in the future also. Because these changes are probably not very clear, why they are being done in this format. Issue is that I can't link all the history behind this in every PR. Obviously "we" don't know this 😄

Just one of the too many examples: #14835

The No.1 problematic issue with this was where macro was defined to 0 or 1 and it was used in the code like this:

#define HAVE_SOMETHING 0

#ifdef HAVE_SOMETHING
...
#endif 

Or the other annoyance on certain systems where -Wundef warnings fill the build log (this is unfortunately still not fixed all the way because of the timelib library and one YYDEBUG being defined incorrectly) but it was done from ~thousands of warnings to only about 50 :)

#undef HAVE_SOMETHING

#if HAVE_SOMETHING
...
#endif

That's why a simple help text in this format follows the current Autoconf AC_DEFINEs. For example, when a macro has two states of undefined/defined (or undefined/defined to 1):

Define to 1 if ...
# or
Define if ...

Where macros having states 0/1:

Define to 1 if ..., and to 0 if not.

This makes it most clear what is happening behind the automatic header file generation. I think that these feature macros are simplest to use in the undefined/defined states, considering the inconsistent code writing possibility.

In CMake for example, this is another story because the configuration template makes it more clear what the macro states are:

#cmakedefine HAVE_SOMETHING /* This will be either defined or undefined */
#cmakedefine01 HAVE_SOMETHING_ELSE /* This will be either 0 or 1 */

[cmb69]

Ah, thanks for the explanation! I now understand that the verbose desciption makes sense.

[petk]

Merge coming up then.. For the other ext/gd macros the issue is that they are a bit more complex to change overall. There is a AC_DEFUN used there so these need to be done differently if this will be changed. I'll check what to do about those. It would be actually good to sync those also a bit, yes.

[thg2k]

@petk Sorry, I don't understand. You said that if the usage is #ifdef HAVE_SOMETHING the help text should say "Define if..", while if #if HAVE_SOMETHING == 1 the text should say "Define to 1 if ..., and to 0 if not.", yet you write "Define to 1 if" for these macros, that are used as def/undef.

Also could we have a general custom m4 macro with the template "Define to 1 if [text], and to zero otherwise" and "Define if [text]", something like PHP_DEFINE_TOGGLE() or PHP_DEFINE_VALUE() ?

[thg2k]

Edit: The reason I ask about the macro with the text template is to avoid repeating the same over and over, in fact in this patch there is a typo.

[petk]

Also could we have a general custom m4 macro with the template "Define to 1 if [text], and to zero otherwise" and "Define if [text]", something like PHP_DEFINE_TOGGLE() or PHP_DEFINE_VALUE() ?

There already was attempt to make this "simpler" with macro PHP_DEF_HAVE. But it is written a bit poorly using the AC_DEFINE instead of AC_DEFINE_UNQUOTED (when there are shell variables used the latter should be used according to Autoconf documentation). IMHO, the default Autoconf macros are still more intuitive when writing M4 code and they are default Autoconf way with at least some manual available. Having a custom macro for PHP specific case will be actually more difficult to use. And it also directs people to write one sentence documentation what some macro is about.

Regarding the Define to 1 ... vs Define if ... I'm onto resolving this also. I'm suspecting that this is some relic of the prehistoric compilers where they had issues with adding compilation flags using this format gcc -DHAVE_SOMETHING vs. gcc -DHAVE_SOMETHING=1. Because, for example, the AC_CHECK_FUNCS([foobar]) adds this verbatim in the configuration header:

/* Define to 1 if you have the 'foobar' function. */
#define HAVE_FOOBAR 1

And the HAVE_FOOBAR is either undefined or defined to 1.

Yes, I agree that this is a bit inconsistent for now.

[cmb69]

[…], in fact in this patch there is a typo.

This patch looks correct to me. The usage is

php-src/ext/gd/php_gd.h

Line 24 in d6a75e1

#if defined(HAVE_LIBGD) || defined(HAVE_GD_BUNDLED)

So it checks whether the macros are defined or not (not their value).

[petk]

[…], in fact in this patch there is a typo.

This patch looks correct to me. The usage is

php-src/ext/gd/php_gd.h

Line 24 in d6a75e1

#if defined(HAVE_LIBGD) || defined(HAVE_GD_BUNDLED)

So it checks whether the macros are defined or not (not their value).

Ah, yes there was a typo... Typo fixed via 11094d5 Yes, there was "Define to 1 gd extension...".

[petk]

Just for the info here because it bothers me too but I can't do much about this at the moment in Autotools build system. All modern C code can use today this style of macros: undefined/defined, of course (or defined to 0/1). This particular style when some macro is either undefined or defined to 1 is there mostly for clarity that something is enabled. And it was easier to use in C code like this:

#define HAVE_FUNCTION 1

#if HAVE_FUNCTION
...

But this is not recommended style of C coding in this particular case. At least not with the current compilers and standards. The HAVE_FUNCTION if undefined will resolve to 0 by the compiler (in most cases, however on some configurations there will be also -Wundef warnings emitted). Because compiler expects that HAVE_FUNCTION would be defined to 0 by the user/build system.

We'll unfortunately have to go here with this style of undefined/defined to 1 for the time being for Autotools AC_DEFINEd preprocessor macros. And the fact that existing AC_DEEFINEs already define these macros to 1. BTW, I was consistent here as much as possible (see the HAVE_PS_STRINGS macro :) ). And in the code these macros should always be used with #ifdef or #if defined or similar. Because there are multiple build systems and IMHO it is easier to "explicitly" enable some feature than to always define the feature macro to either 0 or 1 on all build systems. I hope this will become a bit easier in the future.

If you understand my explanation here.

[cmb69]

Yes, @petk, I understand that we need to stick with the mess for a while for BC reasons. :)