FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

GH-131296: fix clang-cl warning on Windows in socketmodule.c by chris-eibl · Pull Request #131821 · python/cpython · GitHub

/ cpython Public

GH-131296: fix clang-cl warning on Windows in socketmodule.c - #131821

Merged
zooba merged 3 commits into
python:mainfrom
chris-eibl:fix_clangcl_socketmodule
Jun 9, 2025
Merged

GH-131296: fix clang-cl warning on Windows in socketmodule.c#131821
zooba merged 3 commits into
python:mainfrom
chris-eibl:fix_clangcl_socketmodule

Conversation

chris-eibl commented Mar 28, 2025
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

Comment thread Modules/socketmodule.c
#ifdef MS_WINDOWS
if (optname == SIO_TCP_SET_ACK_FREQUENCY) {
int dummy;
DWORD dummy;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

fix warning : incompatible pointer types passing 'int *' to parameter of type 'LPDWORD' (aka 'unsigned long *') [-Wincompatible-pointer-types]

Comment thread Modules/socketmodule.c Outdated
PyThread_acquire_lock(netdb_lock, 1);
#endif
SUPPRESS_DEPRECATED_CALL
_Py_COMP_DIAG_PUSH

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The rest of the PR cares about deprecation warnings like

..\Modules\socketmodule.c(6070,9): warning : 'gethostbyname' is deprecated:
Use getaddrinfo() or GetAddrInfoW() instead or define _WINSOCK_DEPRECATED_NO_WARNINGS
to disable deprecated API warnings [-Wdeprecated-declarations]

https://github.com/python/cpython/actions/runs/14044831663/job/39366560293?pr=131690#step:4:183

using the already existing infrastructure _Py_COMP_DIAG_PUSH et al. from pyport.h, because clang-cl unfortunately does not (yet) respect

# define SUPPRESS_DEPRECATED_CALL __pragma(warning(suppress: 4996))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is there another way to spell the pragma that will work? The idea of pyport.h is to deal with compiler differences as much as possible, so it's the place to handle it if the original macro can be made to work.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

clang-cl does not understand suppress, hence I need to push and pop and thus used the existing infrastructure from pyport.h (warning(disable: 4996))

cpython/Include/pyport.h

Lines 308 to 311 in 0045100

#elif defined(_MSC_VER)
#define _Py_COMP_DIAG_PUSH __pragma(warning(push))
#define _Py_COMP_DIAG_IGNORE_DEPR_DECLS __pragma(warning(disable: 4996))
#define _Py_COMP_DIAG_POP __pragma(warning(pop))

Likewise, AFAIK, there is no supress in gcc / "regular" clang, so we need to always push / pop there.

Copy link
Copy Markdown
Member Author

I am pretty sure that failing of Ubuntu (free-threading) / build and test (ubuntu-24.04) (pull_request) is unrelated to the PR.

python-cla-bot Bot commented Apr 6, 2025

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

Copy link
Copy Markdown
Member Author

@zooba May I ask you for a review?

Comment thread Modules/socketmodule.c Outdated
Comment on lines 6173 to 6174
_Py_COMP_DIAG_POP
#endif /* HAVE_GETHOSTBYNAME_R */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

nit: Two space indents of preprocessor directives might be uncommon in CPython except when a line starts with # .

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I indented everywhere by two spaces to easier spot the macros, but I will happily indent by four spaces if this is preferred.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'd prefer to avoid thinking of the intention when reading, so +1 for four spaces.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done. I had a look at the other occurrences in the code base: either no indentation or like the code they guard.

I switched to the latter.

zooba commented Jun 9, 2025

Copy link
Copy Markdown
Member

LGTM. I'll merge after CI is done unless something else comes up (or if someone else sees this and can merge, go ahead)

zooba merged commit cc8e6d2 into python:main Jun 9, 2025
chris-eibl deleted the fix_clangcl_socketmodule branch June 9, 2025 16:48
hunterhogan added a commit to hunterhogan/cpython that referenced this pull request Mar 14, 2026
chris-eibl added the needs backport to 3.14 bugs and security fixes label Mar 23, 2026

Copy link
Copy Markdown

Thanks @chris-eibl for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 23, 2026
…GH-131821)

(cherry picked from commit cc8e6d2)

Co-authored-by: Chris Eibl <138194463+chris-eibl@users.noreply.github.com>

bedevere-app Bot commented Mar 23, 2026

Copy link
Copy Markdown

GH-146339 is a backport of this pull request to the 3.14 branch.

bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Mar 23, 2026
ambv pushed a commit that referenced this pull request Mar 23, 2026
…1821) (GH-146339)

(cherry picked from commit cc8e6d2)

Co-authored-by: Chris Eibl <138194463+chris-eibl@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL