gh-131296: Fix clang-cl warning on Windows in posixmodule.c - #133142
Conversation
Sorry, something went wrong.
auvipy
left a comment
There was a problem hiding this comment.
looks good. do we also need to update relevant test for this change?
Sorry, something went wrong.
|
Generally not. We only fix some compilation warnings and did not make obviously change. can refer to #131296 |
Sorry, something went wrong.
| 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)) { |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
sizeof doesn't work in preprocessor checks anyway.
Sorry, something went wrong.
There was a problem hiding this comment.
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?
Sorry, something went wrong.
|
@zooba Could you review and merge it, thank you! |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Clang-cl detects that on 32-bit builds the variable is always smaller than the value. But since the same code is used for other architectures, we can't just _fix_ it. This cast avoids the tautological-constant-out-of-range-compare warning.
Only 1 warning in posixmodule.c
Please add skip news label :)