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

Fixed some warnings on MinGW target when compiling Socket.cpp. #1152

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MAJigsaw77
Copy link
Contributor

This PR fixes the following build warnings:

  • Redefinition of _WIN32_WINNT: The macro _WIN32_WINNT was being redefined in Socket.cpp. A conditional check has been added to prevent this.

  • Ignored dllimport attribute: Warnings related to the dllimport attribute for inet_pton_func and inet_ntop_func were addressed by removing the unnecessary WINSOCK_API_LINKAGE.

@skial skial mentioned this pull request Sep 7, 2024
1 task
@MAJigsaw77
Copy link
Contributor Author

By looking at this issue #574 maybe something might break without WINSOCK_API_LINKAGE.

@MAJigsaw77
Copy link
Contributor Author

@hughsando Can i get a review on this pr?

@tobil4sk
Copy link
Member

tobil4sk commented Sep 8, 2024

Also should be investigated whether it is still necessary to set _WIN32_WINNT

@tobil4sk
Copy link
Member

The unnecessary WINSOCK_API_LINKAGE seems to have been copied and pasted from the previous code, where the loading relied on WINSOCK_API_LINKAGE rather than GetProcAddress as it does now: b08a79d

Here is a similar example from microsoft documentation of using GetProcAddress (without WINSOCK_API_LINKAGE): https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-getprocaddress#remarks

_WIN32_WINNT was added in a commit intending to fix #569. The issue links to this stackoverflow post, which says:

You may need to set _WIN32_WINNT to a code representing the minimum supported Windows version. That enables the APIs that are only supported in the more recent versions of Windows. MSVC does this for you, MinGW does not, but it is easy to fix.

Mingw-w64 now defines _WIN32_WINNT by default, so maybe this is no longer necessary. It is safe to keep if it is wrapped in an #ifndef guard as in this patch.

So these changes seem to be correct.

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.

2 participants