-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Zend/Zend.m4: use AX_APPEND_COMPILE_FLAGS #10642
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
Conversation
The new macro looks nice.
What's the reason for removing the lines for:
-Wlogical-op
-Wformat-truncation
-Wstrict-prototypes
?
What's the reason for removing the lines for:
I didn't remove them - I rewrote them with AX_APPEND_COMPILE_FLAGS which allows multiple flags in one line, look:
+ AX_APPEND_COMPILE_FLAGS([-Wduplicated-cond -Wlogical-op -Wformat-truncation -Wstrict-prototypes],, [-Werror])
appreciate the simplification.
New *.m4 files need to be added also to the
scripts/phpize.in
which becomes the phpize shell script for PECL extensions.
https://github.com/php/php-src/blob/master/scripts/phpize.in
762a0d0 to
4c467d6
Compare
New *.m4 files need to be added also to the
scripts/phpize.in
Amended.
And probably the same can be done for the AX_CHECK_COMPILE_FLAG call in sapi/fuzzer/config.m4?
And probably the same can be done for the AX_CHECK_COMPILE_FLAG call in sapi/fuzzer/config.m4?
Yes, but I didn't do that yet because this instance then adds the result to two variables:
Lines 33 to 38 in 8995f60
AX_APPEND_COMPILE_FLAGS adds the flags only to the *FLAGS variable of the current language, i.e. C (CFLAGS).
The call in sapi/fuzzer/config.m4 checks only the capabilities of the C compiler and then adds the flag to both, which is incorrect, because it did not check whether the C++ compiler supports the flag.
For now, I wanted to leave it that way, to be improved later. The rest of PHP is inconsistent as well - it checks warning flags but adds it only to CFLAGS. See #10663 (comment) for some fallout of this existing bug. I will address all of these problems in a new PR, because I feel it is out of this PR's scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be sorted alphabetically. Autoconf includes these at the beginning anyway regardless when calling some macro.
For the phpize script I'm not sure what would be the best approach. On one hand these additional m4 files maybe even shouldn't be included for PECL extensions. If they will be included, then what can also be done instead is something like this:
FILES_BUILD="*.m4 config.guess config.sub gen_stub.php ltmain.sh Makefile.global shtool"And the phpize.m4 also have the m4_include() lines at the beginning which can be synced with configure.ac.
No description provided.