Skip to content

Conversation

@mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Apr 21, 2023

extracted from #8479

discussed in #8479 (comment)

introduced in 08b732f

@mvorisek
Copy link
Contributor Author

CI failure is unrelated

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Is this only happening on 64bits? because this change seems weird and it would make more sense to have an #if defined(PHP_WIN32) || defined(_WIN64)

@mvorisek
Copy link
Contributor Author

Is this only happening on 64bits? because this change seems weird and it would make more sense to have an #if defined(PHP_WIN32) || defined(_WIN64)

yes, on x64 only, x86 need this PR/fix

@mvorisek
Copy link
Contributor Author

@nielsdos can you please approve based on discussion in #8479 (comment)

@ndossche
Copy link
Member

ndossche commented May 2, 2023

I'll take a look later this week with my Windows testing VM.

@ndossche
Copy link
Member

ndossche commented May 7, 2023

I build this in my Windows VM for x86 and I think this looks right. Thanks. I do however think that targeting master is more appropriate: there seems to be an (unwritten?) rule that compiler warnings that don't cause bugs or errors should target master.

Is this only happening on 64bits? because this change seems weird and it would make more sense to have an #if defined(PHP_WIN32) || defined(_WIN64)

So here's something that always confused me too: PHP_WIN32 just means Windows (for BC reasons), it is set both on 32-bit and 64-bit. Want to detect Windows 64-bit? Then use _WIN64...
So this PR only changes the behaviour for 32-bit Windows.

@mvorisek mvorisek changed the base branch from PHP-8.1 to master May 7, 2023 21:12
@mvorisek
Copy link
Contributor Author

mvorisek commented May 7, 2023

Thank you for your post.

The PR is retargeted to master.

@ndossche ndossche merged commit 37e6594 into php:master May 7, 2023
@ndossche
Copy link
Member

ndossche commented May 7, 2023

Thanks!

@mvorisek mvorisek deleted the fix_gmp_msvc branch May 7, 2023 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants