Skip to content
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

Remove check for time.h and HAVE_TIME_H #11726

Merged
merged 1 commit into from Jul 20, 2023
Merged

Remove check for time.h and HAVE_TIME_H #11726

merged 1 commit into from Jul 20, 2023

Conversation

petk
Copy link
Member

@petk petk commented Jul 17, 2023

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.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

LGTM!

@petk
Copy link
Member Author

petk commented Jul 18, 2023

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
@petk
Copy link
Member Author

petk commented Jul 19, 2023

@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...

@iluuu1994
Copy link
Member

@petk That confuses me a bit. Wouldn't HAVE_TIME_H still be defined on Windows and the header thus still included?

@petk
Copy link
Member Author

petk commented Jul 19, 2023

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 win32/build/config.w32.h.in if that would be needed. And in that case it would fail on Windows ZTS because of the TIME_H constant in time.h and in win32/time.h. Theoretically, if configure would be executed on Windows system with ZTS enabled it would cause similar errors (not in formal parameter list, missing ')' before...)

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 win32/time.h could be refactored a bit further. It is solution for the sys/time.h which isn't available on Windows.

And with this patch it makes it more clear where time.h is needed and where shouldn't be. Something like that.

@iluuu1994
Copy link
Member

Thanks for your explanation! Your change looks good then!

@petk petk merged commit b132b7a into php:master Jul 20, 2023
12 checks passed
@petk petk deleted the patch-time-h branch July 20, 2023 07:38
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.

None yet

2 participants