gh-124064: Fix some -Wconversion warnings #124174
Conversation
vstinner
requested review from
Fidget-Spinner,
lysnikolaou and
pablogsal
as code owners
| #else | ||
| PyGC_Head *gc = _Py_AS_GC(op); | ||
| gc->_gc_prev &= ~_PyGC_PREV_MASK_FINALIZED; | ||
| gc->_gc_prev &= ~(uintptr_t)_PyGC_PREV_MASK_FINALIZED; |
There was a problem hiding this comment.
I think it might be better to make the _PyGC_PREV_MASK_FINALIZED literal unsigned.
There was a problem hiding this comment.
I tried #define _PyGC_PREV_MASK_FINALIZED 1U but the code got miscompiled and failed.
There was a problem hiding this comment.
Ugh... you're right. It won't get sign extended in that case.
There was a problem hiding this comment.
I wrote a different fix in the following change: #124177
-#define _PyGC_PREV_MASK_FINALIZED 1
+#define _PyGC_PREV_MASK_FINALIZED ((uintptr_t)1)It seems to work like that. I also prefer to fix the constant declaration rather than having to cast the constant every time.
There was a problem hiding this comment.
FYI, I started a discussion about -Wsign-conversion in the #gsoc-security-project Discord channel. It seems to me that adding these explicit casts in many places to address -Wsign-conversion does not make the code less bug prone and probably does the opposite.
In this case, we now have a tighter coupling between the _PyGC_PREV_MASK_FINALIZED and the type of _gc_prev and _gc_next. That seems worse to me than before where we relied on the implicit sign conversion.
For example, if we hypothetically changed _gc_prev to uint64_t, the code would now be incorrect.
| PyStackRef_AsPyObjectBorrow(_PyStackRef stackref) | ||
| { | ||
| PyObject *cleared = ((PyObject *)((stackref).bits & (~Py_TAG_BITS))); | ||
| PyObject *cleared = ((PyObject *)((stackref).bits & (~(uintptr_t)Py_TAG_BITS))); |
3 participants
Uh oh!
There was an error while loading. Please reload this page.