Skip to content

sapi/fpm: remove use of variable-length arrays #10645

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
Feb 21, 2023
Merged

Conversation

MaxKellermann
Copy link
Contributor

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.

@devnexen devnexen requested a review from bukka February 21, 2023 12:25
@bukka
Copy link
Member

bukka commented Feb 21, 2023

FPM doesn't support Windows so it can require VLA support. I have no issue with with trace and stdio changes but not sure if we want to put procs on heap atm. Will think about but it's a low priority.

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
Copy link
Contributor Author

OK, I removed the fpm_status.c change and will resubmit it later in a new PR, so the fixes for the two accidental VLAs can be merged now.

@Girgias Girgias self-assigned this Feb 21, 2023
@Girgias Girgias merged commit ff2a211 into php:master Feb 21, 2023
@MaxKellermann MaxKellermann deleted the no_vla branch February 21, 2023 17:07
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