-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Remove check for time.h and HAVE_TIME_H #11726
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
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.
LGTM!
There is an error on Windows with ZTS. Then headers need to be included a bit differently I think. It is probably related to WIN32_LEAN_AND_MEAN. |
The `<time.h>` header file is part of the standard C89 headers [1] and on current systems can be included unconditionally. The conditional include based on Windows is there so the win32/time.h can be included on other places when needed. Refs: [1] https://port70.net/~nsz/c/c89/c89-draft.html#4.1.2 [2] https://git.savannah.gnu.org/cgit/autoconf.git/tree/lib/autoconf/headers.m4
@iluuu1994 I've changed this a bit. So, basically on Windows time.h shouldn't be included due to win32/time.h and elsewhere it's fine... |
@petk That confuses me a bit. Wouldn't |
HAVE_TIME_H is in current code defined during build process from configure (that time.h presence in configure.ac). And this has been so far executed only on Linux systems. That's why it is included on Linux and not on Windows. Windows build in this case would need to define it manually in the Yes, it's a bit of a mess in PHP code that's why these hacks. Otherwise, it would be fine to include time.h unconditionally on all platforms. Probably at some point in the future And with this patch it makes it more clear where time.h is needed and where shouldn't be. Something like that. |
Thanks for your explanation! Your change looks good then! |
The
<time.h>
header file is part of the standard C89 headers [1] and on current systems can be included unconditionally.Refs:
[1] https://port70.net/~nsz/c/c89/c89-draft.html#4.1.2 [2] https://git.savannah.gnu.org/cgit/autoconf.git/tree/lib/autoconf/headers.m4
In PHP 7.4 these were already cleaned a bit. Probably we don't need to re-add it in PHP 8.3.