Skip to content

build/php.m4: remove test for integer types #10304

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 13, 2023
Merged

Conversation

MaxKellermann
Copy link
Contributor

These are mandatory in C99, so it's a pointless waste of time to check for them.

(Actually, the fixed-size integer types are not mandatory, but if they are really not available on some theoretical system, PHP's fallbacks won't work either, so nothing is gained from this check.)

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

These are mandatory in C99, so it's a pointless waste of time to check
for them.

(Actually, the fixed-size integer types are not mandatory, but if they
are really not available on some theoretical system, PHP's fallbacks
won't work either, so nothing is gained from this check.)
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why I didn't clean those up when I did the clean-up for a bunch of other checks.

@Girgias Girgias merged commit 7473b86 into php:master Jan 13, 2023
@MaxKellermann MaxKellermann deleted the c99_int branch January 13, 2023 11:53
@@ -26,6 +26,10 @@ PHP 8.3 INTERNALS UPGRADE NOTES
break the build of third-party extensions which relied on this
implementation detail. Those extensions may need to add the missing
#include lines.
* Since version 8, PHP requires a C99 compiler. Configure-time checks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, we do not actually fully C99 compliant compilers, but merely rely on some of its features (e.g. we do not require VLA support, which is optional as of C11). I don't think this needs to be updated, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requiring a C99 compiler doesn't imply that all C99 features are really used - VLA is a special example of a feature that many people nowadays think was a bad idea, and I'm happy PHP doesn't use them.
I compile my projects with -Wvla so GCC kicks me if I accidently use one...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do not require VLA support, which is optional as of C11

@cmb69, turns out PHP does use VLAs!

struct fpm_scoreboard_proc_s procs[scoreboard.nprocs];

char buf[max_buf_size];

char buf[buf_size];

@MaxKellermann
Copy link
Contributor Author

Please provide changes to ext/date/lib/* to upstream; otherwise they'll likely be overwritten by the next update.

@cmb69 that PR was rejected by @derickr: derickr/timelib#141 (comment)

I'm puzzled by his stated reason....

@cmb69
Copy link
Member

cmb69 commented Jan 18, 2023

I've just stumbled upon https://github.com/php-memcached-dev/php-memcached/blob/04a0f72eaafc0ac4ba646b57761b8d7331cdc82a/php_memcached_private.h#L65-L79. Many other extensions may be affected by this removal as well. I'm not suggesting to revert this, but we should have a look at the discussion on derickr/timelib#141.

@MaxKellermann
Copy link
Contributor Author

Yeah, apparently people have been copying code ...

Apparently, extensions are supposed to #include "php.h". If we really care for the utmost source compatibility, what we could do is add some compatibility glue there. Like including a new header php_compat.h which has all those definitions we removed, and maybe even includes errno.h.

Maybe make including php_compat.h optional, with some macro like PHP_NO_COMPAT which can be used to opt out of this, for eager extension authors.

(The "timelib" problem is different; it's a vendored library embedded in PHP which PHP can easily fix in its local copy, but feeding that change back to timelib appears to be difficult.)

@MaxKellermann
Copy link
Contributor Author

Like including a new header php_compat.h

This header already exists - undocumented, but apparently for the same reason I wanted to have it.

@cmb69, what do you think, do you want me to add the HAVE_* macros that were removed by this PR to that header?
What about adding a macro check like PHP_NO_COMPAT to be able to opt out of this header?

MaxKellermann added a commit to CM4all/php-src that referenced this pull request Feb 21, 2023
According to @cmb69, PHP does not require VLA support
(php#10304 (comment)).
VLAs are a bad idea for several reasons, so let's get rid of them.

Two of the VLAs were probably unintended; unlike C++, C doesn't have
the concept of "constant expressions", so an array with a "const"
length is technically still a VLA.  This is fixed by removing the
"const" variable, and using sizeof() instead.
MaxKellermann added a commit to CM4all/php-src that referenced this pull request Feb 21, 2023
According to @cmb69, PHP does not require VLA support
(php#10304 (comment)).
VLAs are a bad idea for several reasons, so let's get rid of them.

Two of the VLAs were probably unintended; unlike C++, C doesn't have
the concept of "constant expressions", so an array with a "const"
length is technically still a VLA.  This is fixed by removing the
"const" variable, and using sizeof() instead.
Girgias pushed a commit that referenced this pull request Feb 21, 2023
According to @cmb69, PHP does not require VLA support
(#10304 (comment)).
VLAs are a bad idea for several reasons, so let's get rid of them.

Two of the VLAs were probably unintended; unlike C++, C doesn't have
the concept of "constant expressions", so an array with a "const"
length is technically still a VLA.  This is fixed by removing the
"const" variable, and using sizeof() instead.
MaxKellermann added a commit to CM4all/php-src that referenced this pull request Feb 22, 2023
According to @cmb69, PHP does not require VLA support
(php#10304 (comment)).
VLAs are a bad idea for several reasons, so let's get rid of them.

This is a continuation of php#10645
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants