GH-131296: fix clang-cl warning on Windows in getpath.c #131594
Conversation
with expression of type 'const char' is always true [-Wtautological-constant-out-of-range-compare]
| @@ -690,7 +690,6 @@ 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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sorry, to be clear I think we probably should make an assertion about the signedness of char earlier on, and remove this assertion.
There was a problem hiding this comment.
Ah - ok, but this will need its own issue and PR.
There was a problem hiding this comment.
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.
|
@FFY00 friendly ping. IMHO this is a very simple one :) |
I think this is a skip news?