Add Werror to CI #56277
Add Werror to CI #56277
Conversation
WillAyd
commented
Dec 1, 2023
WillAyd
commented
- closes ENH: Add -Werror to CI (or at least show warnings) #55982
| @@ -12,7 +12,7 @@ The full license is in the LICENSE file, distributed with this software. | |||
| #include <numpy/ndarraytypes.h> | |||
|
|
|||
| // Scales value inplace from nanosecond resolution to unit resolution | |||
| int scaleNanosecToUnit(npy_int64 *value, NPY_DATETIMEUNIT unit); | |||
| int scaleNanosecToUnit(int64_t *value, NPY_DATETIMEUNIT unit); | |||
There was a problem hiding this comment.
@phofl pointed out on macOS that npy_int64 is a typedef for a long whereas int64_t is a typedef for a long long. Unfortunately referencing these via pointer violates the strict aliasing rule
For now just picked int64_t as it was bigger. I'm not sure in practice that this matters to much, but we do freely mix these uses today
| NPY_DATETIMEUNIT dateUnit = NPY_FR_ns; | ||
| if (PyTypeNum_ISDATETIME(type_num)) { | ||
| is_datetimelike = 1; | ||
| i8date = *(npy_int64 *)dataptr; | ||
| i8date = *(int64_t *)dataptr; |
There was a problem hiding this comment.
This is not a safe cast, but leaving to another PR to fix. The proper way to do this would be via memcpy
There was a problem hiding this comment.
It is likely pedantic but I'm fairly certain this is a strict aliasing violation. Where/when that matters may be up for debate. The memcpy from that comment would seemingly be safer
|
Any feedback on this? Would love to get this one in before any other PRs that touch extensions |
| // value exceeds number of bits that significand can hold | ||
| int64_t value = PyObject_HasAttrString(obj, "_value") | ||
| ? get_long_attr(obj, "_value") | ||
| : (int64_t)total_seconds(obj) * 1000000000LL; |
There was a problem hiding this comment.
Does the int64_t cast apply to total_seconds(obj) first? I think this would truncate e.g. Timedelta(milliseconds=1) to 0 first before multiplying by 1000000000LL?
There was a problem hiding this comment.
Oh nice catch - I think you are right about that. Let me see if we can add a test for that
|
Great to have this back running @WillAyd |
