10000 GH-131296: fix clang-cl warning on Windows in getpath.c by chris-eibl · Pull Request #131594 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

GH-131296: fix clang-cl warning on Windows in getpath.c #131594

8000
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.

Sign up for GitHub

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 3 commits into from
Apr 22, 2025
+1 −1
Merged

GH-131296: fix clang-cl warning on Windows in getpath.c #131594

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a94efc
fix warning : result of comparison of constant 128
chris-eibl Mar 22, 2025
822534f
Merge remote-tracking branch 'origin/main' into fix compiling zlib-ng
chris-eibl Apr 21, 2025
7275f2b
use assert(!(*p & 0x80));
chris-eibl Apr 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Modules/getpath.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,7 +690,7 @@ env_to_dict(PyObject *dict, const char *key, int and_clear)
// Quick convert to wchar_t, since we know key is ASCII
wchar_t *wp = wkey;
for (const char *p = &key[4]; *p; ++p) {
assert(*p < 128);
Copy link
Member Author
@chris-eibl chris-eibl Mar 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

fix warning : result of comparison of constant 128 with expression of type 'const char' is always true [-Wtautological-constant-out-of-range-compare]

We are here in an #ifdef MS_WINDOWS where a char will always be signed, so this assert can safely be deleted.

Copy link
Member

Choose a reason for hiding this comment

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

If you compile with /J char is unsigned. (ref https://learn.microsoft.com/en-us/cpp/cpp/char-wchar-t-char16-t-char32-t?view=msvc-170)

I don't know if that is supported in CPython or used by anyone though, I expect it would be quite rare. Perhaps that should be a compile error?

Copy link
Member

Choose a reason for hiding this comment

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

I would imagine setting /J would break the ABI, so I would imagine that is a good reason to make it a compile error if char is unsigned.

Copy link
Member Author

Choose a reason for hiding this comment

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

Boah, yeah, would have never imagined to think of making char unsigned on Windows, because that would presumably result in a lot of trouble. But since you've mentioned it - if we'd like this to be asserted here (and not a more prominent place like pyport.h using an explicit static_assert, etc to fail at compile time), then we could keep it. It just won't do its job well, since presumably this will not fire reliably here.

But then we'd have to silence the warning: maybe with a pragma diagnostic?

Im still +0 for just removing this assert.

@zooba WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, to be clear I think we probably should make an assertion about the signedness of char earlier on, and remove this assertion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah - ok, but this will need its own issue and PR.

Copy link
Member

Choose a reason for hiding this comment

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

We still need the assertion if it's signed, it just has to check that it's not negative.

Perhaps asserting !(*p & 0x80) is the better fix? It's unlikely that someone is going to start providing Unicode characters elsewhere in the source file, but that is the point of the check.

assert(!(*p & 0x80));
*wp++ = *p;
}
*wp = L'\0';
Expand Down
Loading
0