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

Use "_WIN32" macro #2905

Closed
wants to merge 6 commits into from

Conversation

7 participants
@cup
Copy link

commented Nov 5, 2017

PHP_WIN32 can be replaced, as it was introduced as a duplicate of
ZEND_WIN32 [1]:

#define PHP_WIN32 ZEND_WIN32

ZEND_WIN32 can be replaced, as it was introduced as a concise way to check for
Windows [2]:

#define ZEND_WIN32 ((defined(WINNT) && WINNT) || (defined(WIN32) && WIN32))

WINNT was introduced as a way to check for Windows NT [3]:

#if WIN32|WINNT
  // Get build numbers for Windows NT or Win95
  if (dwVersion < 0x80000000){
    php3_os="WINNT";
  } else {
    php3_os="WIN32";
  }
#else
  php3_os=PHP_OS;
#endif

Windows NT [4] is no longer supported by PHP. "_WIN32" [5] is the documented
macro for Windows. Also I removed "_WINDOWS" because it was defined but never
used, and replaced WINDOWS as well because it is also not documented.

[1] http://github.com/php/php-src/commit/f452c77
[2] http://github.com/php/php-src/commit/bc5c9d8
[3] http://github.com/php/php-src/commit/573b460
[4] http://wikipedia.org/wiki/Windows_NT_4.0
[5] http://docs.microsoft.com/cpp/preprocessor/predefined-macros

cup referenced this pull request Nov 5, 2017

Add new (optional!) win32 build infrastructure.
Will follow up to internals@ shortly.

@nikic nikic requested a review from weltling Nov 5, 2017

@weltling
Copy link
Contributor

left a comment

The most of concern I have that this change can affect merging from lower branches also for unrelated platforms. At least, PHP_WIN32 and co. definitions should not've been removed. There's a lot of code outside core that would become invalid immediately without these defs. Sometimes these macros are used also in the patched dependencies, fe see the AppVeyor failing on snmp headers. Of course nowadays _WIN32 were enough otherwise, to tell the common platform, lets see what the future brings.

All in all, I'd be reluctant on this patch. While the change is pure cosmetic, the troubles it can cause are something no one would need.

Thanks

@KalleZ

This comment has been minimized.

Copy link
Member

commented Nov 6, 2017

I'm in the same boat here as @weltling, its a purely cosmetic change. While I do understand that it will help with the mingw64 support we're working on in the bug report, this will however just be a slight annoyance to change for anyone working on the core and extensions

herumi and others added some commits Nov 3, 2017

Use "_WIN32" macro
PHP_WIN32 can be replaced, as it was introduced as a duplicate of
ZEND_WIN32 [1]:

    #define PHP_WIN32 ZEND_WIN32

ZEND_WIN32 can be replaced, as it was introduced as a concise way to check for
Windows [2]:

    #define ZEND_WIN32 ((defined(WINNT) && WINNT) || (defined(WIN32) && WIN32))

WINNT was introduced as a way to check for Windows NT [3]:

    #if WIN32|WINNT
      // Get build numbers for Windows NT or Win95
      if (dwVersion < 0x80000000){
        php3_os="WINNT";
      } else {
        php3_os="WIN32";
      }
    #else
      php3_os=PHP_OS;
    #endif

Windows NT [4] is no longer supported by PHP. "_WIN32" [5] is the documented
macro for Windows. Also I removed "_WINDOWS" because it was defined but never
used, and replaced WINDOWS as well because it is also not documented.

[1] http://github.com/php/php-src/commit/f452c77
[2] http://github.com/php/php-src/commit/bc5c9d8
[3] http://github.com/php/php-src/commit/573b460
[4] http://wikipedia.org/wiki/Windows_NT_4.0
[5] http://docs.microsoft.com/cpp/preprocessor/predefined-macros
@cup

This comment has been minimized.

Copy link
Author

commented Nov 6, 2017

AppVeyor working now - I left the other changes but did not change win32/build/confutils.js

@weltling

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2017

@svpenn i'd really advise against this patch, as mentioned above, until there are strong arguments besides macro naming.

Thanks.

@Sebbyastian

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2017

Apologies if this seems out of place, but I feel the need to offer some input. I don't think this is merely cosmetic, but also a semantically sound suggestion.

According to a number of official sources, _WIN32 is used to select x86 and amd64. I only searched briefly, but I couldn't find any official mention at all of WINNT. That's not good!

One of my biggest reasons to pick on PHP is that the implementation makes subtle (probably mistaken) use of implementation-defined (and undefined) behaviours which might result in variations between behaviour from configuration to configuration, perhaps even when the same OS is being used.

It might be wise to use those preprocessor macros which the Microsoft website documents for Microsoft systems, as the others have no guarantee at all regarding functionality and could (theoretically, however unlikely the probability) change overnight without a word.

@krakjoe

This comment has been minimized.

Copy link
Member

commented Nov 26, 2017

@@ -1170,7 +1170,7 @@ diff -u libmagic.orig/compress.c libmagic/compress.c
# endif /* HAVE_SIG_T */
#endif
-#if !defined(__MINGW32__) && !defined(WIN32)

This comment has been minimized.

Copy link
@Sebbyastian

Sebbyastian Nov 26, 2017

Contributor

__MINGW32__ appears to target a compiler infrastructure, including standard library and headers which may or may not (probably not) contain <sys/ioctl.h>, where-as my Windows 7 system with MSYS2 installed reports the presense of <sys/ioctl.h>. Is it possible that you might have confused a compiler-related macro with an OS-related macro?

@Sebbyastian

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2017

The argument that non standard definitions may be removed would be more powerful if we hadn't been using them forever

Your own line of reasoning would also be more powerful if you weren't using a standardised language.

They will not be changed for exactly the same reason we cannot merge this patch.

If that's your only issue, it seems like it could be a few simple fixes to get this patch merging. I'll finish identifying the syntax errors that the changes caused. I'm fairly confident the semantic differences between the unofficial WINNT and the official _WIN32 will be minimal, if anything, once these syntax errors are fixed.

@Sebbyastian

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2017

@svnpenn I've left a number of comments for you to check out. I think you should add the mingw check back in, as I'm pretty sure that ancient thing will still be used by anyone who hasn't found more up-to-date, greener pastures. You might also be interested in analysing the difference between the definition of _WIN32 on various systems, such as here, where it becomes obvious that some systems indeed will render code such as #if _WIN32 as a constraint violation (hence the CI fails?). It should be pretty straight forward what might need to be done, but nonetheless I'm happy to help if you have any questions.

@Sebbyastian

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2017

I also note a boolean-compatible value specified for ZEND_WIN32, whilst just a blank macro from _WIN32 (and WIN32)... Did you miss this file as you were hunting down and eliminating ZEND_WIN32, perhaps?

@Sebbyastian

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2017

@svnpenn Ownership is stubborn, but will accept this commit when he sees better portability. Don't give up... or do... whatever, I don't care. PHP's nothing special, anyway.

@cup

This comment has been minimized.

Copy link
Author

commented Nov 27, 2017

@Sebbyastian not sure where you are getting that information - the other comments in this thread have made it clear that this patch wont be accepted even with passing tests - because of extensions

@weltling

This comment has been minimized.

Copy link
Contributor

commented May 5, 2018

@svnpenn for historical reasons, it's preferable to keep the existing macros. I'm closing this PR therefore. Thanks for your work!

@weltling weltling closed this May 5, 2018

@cup cup referenced this pull request Jun 9, 2018

Open

Coordinate porting to Windows #5430

12 of 21 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.