Skip to content

Fix gmp_long/gmp_ulong typedef warning on Windows x86 #11112

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

Merged
merged 3 commits into from
May 7, 2023

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)

@nielsdos
Copy link
Member

nielsdos commented May 2, 2023

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

@nielsdos
Copy link
Member

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

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

nielsdos 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