Skip to content

Php8 comp #528

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.

Sign up for GitHub

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 3 commits into from
Jan 31, 2023
Merged

Php8 comp #528

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee77de5
PHP 8 compatibility Windows
Dec 13, 2022
f05d4fa
PHP 8 compatibility Windows
Dec 22, 2022
8caadef
PHP_VERSION_ID instead of _MSC_VER
lifenglsf Jan 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions php_memcached_private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,11 @@
#include <ext/standard/basic_functions.h>

#ifdef PHP_WIN32
# if PHP_VERSION_ID >= 80000
# include "php_stdint.h"
#else
# include "win32/php_stdint.h"
#endif
#if (defined(_MSC_VER) && _MSC_VER<=1920)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is defined by the Microsoft C compiler version. Can you point to a document explaining why this is the right variable to use to switch which header to include?

Copy link
Contributor Author

@lifenglsf lifenglsf Jan 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

for this commit,php removed php_stdint.h file

Copy link
Contributor

Choose a reason for hiding this comment

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

Found it: https://github.com/php/php-src/blob/PHP-8.2/UPGRADING.INTERNALS#L26

But I still don't understand why the MSC_VER is the right way to determine whether to use the win32 header variant. Isn't the issue here that the PHP version needs to be checked for >= 80200?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's probably best to check for the PHP version only; in theory, <stdint.h> might not be available (i.e. for very old MSVC/SDKs), but in practice this can be ignored.

Copy link
Contributor Author

@lifenglsf lifenglsf Jan 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

reference this pull request

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I just realize that this is actually about win32/php_stdint.h which is removed as of PHP 8.0.0 (I thought it is about main/php_stdint.h which is removed as of PHP 8.2.0 only). So, I don't understand why there should be any change at all. Apparently, standard Windows builds for PHP 8.0 and 8.1 are successful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to less if else condition

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, simplification is nice, and compatibility with PHP 8.2.0 is important. But why not:

 php_memcached_private.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/php_memcached_private.h b/php_memcached_private.h
index b4b1115..2d67e36 100644
--- a/php_memcached_private.h
+++ b/php_memcached_private.h
@@ -48,11 +48,11 @@
 #include <ext/standard/basic_functions.h>
 
 #ifdef PHP_WIN32
-  #  if PHP_VERSION_ID >= 80000
-  #  include "php_stdint.h"
-#else
-# include "win32/php_stdint.h"
-#endif
+# if PHP_VERSION_ID >= 80000
+#  include <stdint.h>
+# else
+#  include "win32/php_stdint.h"
+# endif
 #else
 /* Used to store the size of the block */
 #  if defined(HAVE_INTTYPES_H)

This is basically the same as your suggestion, but checking for PHP_VERSION instead of _MSC_VER.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok,good suggestion.i'll modify it.

Copy link
Contributor

Choose a reason for hiding this comment

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

thank you, I'm keeping an eye out for the PR update!

#include "win32/php_stdint.h"
#else
#include <stdint.h>
#endif
#else
/* Used to store the size of the block */
# if defined(HAVE_INTTYPES_H)
Expand Down