Skip to content

gh-131296: Fix clang-cl warning on Windows in posixmodule.c #133142

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 2 commits into from
May 5, 2025

Conversation

Wulian233
Copy link
Contributor

@Wulian233 Wulian233 commented Apr 29, 2025

Only 1 warning in posixmodule.c

..\Modules\posixmodule.c(4193,17): warning : result of comparison of constant 4611686018427387903 with expression of ty
pe 'DWORD' (aka 'unsigned long') is always true [-Wtautological-constant-out-of-range-compare] 

Please add skip news label :)

Copy link

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

looks good. do we also need to update relevant test for this change?

@Wulian233
Copy link
Contributor Author

Generally not. We only fix some compilation warnings and did not make obviously change. can refer to #131296

@@ -4190,7 +4190,7 @@ posix_getcwd(int use_bytes)
terminating \0. If the buffer is too small, len includes
the space needed for the terminator. */
if (len >= Py_ARRAY_LENGTH(wbuf)) {
if (len <= PY_SSIZE_T_MAX / sizeof(wchar_t)) {
if ((Py_ssize_t)len <= PY_SSIZE_T_MAX / sizeof(wchar_t)) {
Copy link
Member

Choose a reason for hiding this comment

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

This indeed fixes the warning. IMHO, that's the most pragmatic fix. We could be fancy and do some #if on sizeof(Py_ssize_t), etc, but I don't think it is worth the hassle.

Copy link
Member

Choose a reason for hiding this comment

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

sizeof doesn't work in preprocessor checks anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I meant something like we do with SIZEOF_SIZE_T, but did not want to elaborate about it in great detail, because IMHO it isn't worth the churn?

@Wulian233
Copy link
Contributor Author

@zooba Could you review and merge it, thank you!

@zooba
Copy link
Member

zooba commented May 5, 2025

It's a bit of an annoying warning, because they'd be justified in still warning even with the cast (since it still can't ever be larger than that value for a given architecture), but there's nothing more we can do about it. If this cast ever stops suppressing the warning, then we probably just have to disable the warning.

@zooba zooba merged commit b6c2ef0 into python:main May 5, 2025
45 checks passed
@Wulian233 Wulian233 deleted the posix-warn branch May 5, 2025 22:20
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.

6 participants