Skip to content

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.

Copy link
Member

@tobil4sk tobil4sk left a comment

Choose a reason for hiding this comment

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

As explained a both, these solutions to these warnings make sense and would be useful for cleaning up the compilation output with mingw.

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