MAINT: default to C11 rather than C99, fix most build warnings with Clang 14 #25072
Conversation
There was a problem hiding this comment.
LGTM, but that arm typo does not look like a typo, I can see it leading to uninitialized unused variables though.
I assume nobody will care about C11 vs. C99. If anyone does (and actually has problems with those redefinitions), I suspect they can untangle the imports to fix it with some time.
| /* At least GCC 9.2 issues spurious warnings for arg2 below. */ | ||
| #pragma GCC diagnostic push /* matching pop after function and repeat */ | ||
| #pragma GCC diagnostic ignored "-Wmaybe-uninitialized" | ||
| #endif |
There was a problem hiding this comment.
If CI doesn't annoy us anymore, great that we don't need to add the unnecessary init.
| return npyv_mul_@sfx@(hypot, larger); | ||
| } | ||
| #endif // VECTOR | ||
| /**end repeat**/ |
There was a problem hiding this comment.
For those not reading the commit message carefully: A copy of this is in loops_unary_complex.dispatch.c.src where it is actually used.
|
One CI failure too, with Clang-cl on Windows: Probably due to |
|
Right, I just rebased gh-25044 I suspect that might help. EDIT: Well, actually, I am not sure how |
|
The Should we submit a bug report there? |
|
Good catch @lysnikolaou! Yes please! If you could take that one on, that'd be helpful. |
|
The Cython bug has been fixed in cython/cython#5809. Should we try Windows CI out with cython master? |
I tried this on my fork, CI passes now with both Clang-cl and MSVC with the Cython |
|
I put a backport label on this, seems pretty straight forward. |
13ac7be to
1960458
Compare
|
This is happy now that the Cython issue for C11 was fixed and released in 3.0.6
I'd be fine either way with backporting the MAINT commits yes or no. I'd prefer not to backport the BLD ones though. |
…t warnings Also a simple fix for a `-Wmaybe-uninitialized` warning in `special_integer_comparisons.cpp` (GCC isn't smart enough to understand the `default` part of the switch statement without the added line).
…arning
That warning was, with clang on macOS:
```
../numpy/_core/src/npysort/selection.cpp:529:15: warning: result of comparison of constant 1 with expression of type 'NPY_SELECTKIND' is always false [-Wtautological-constant-out-of-range-compare]
if (which >= NPY_NSELECTS) {
```
The warning was: ``` [2/3] Compiling C object numpy/_core/_multiarray_umath.cpython-39-darwin.so.p/src_multiarray_item_selection.c.o ../numpy/_core/src/multiarray/item_selection.c:2371:1: warning: unused function 'count_nonzero_bytes_384' [-Wunused-function] count_nonzero_bytes_384(const npy_uint64 * w) ```
In effect we were already using C11 for a long time, because that's
where `complex.h` and complex types became optional rather than
mandatory. Also, there were several places where we used typedef
redefinitions, which is a C11 feature, as indicated by build warnings
like these:
```
In file included from ../numpy/_core/src/multiarray/common.h:6:
../numpy/_core/include/numpy/ndarraytypes.h:308:31: warning: redefinition of typedef 'NpyAuxData' is a C11 feature [-Wtypedef-redefinition]
typedef struct NpyAuxData_tag NpyAuxData;
^
../numpy/_core/src/umath/loops.h.src:37:31: note: previous definition is here
typedef struct NpyAuxData_tag NpyAuxData;
^
In file included from ../numpy/_core/src/multiarray/array_method.h:14:
../numpy/_core/include/numpy/_dtype_api.h:108:3: warning: redefinition of typedef 'PyArrayMethod_Context' is a C11 feature [-Wtypedef-redefinition]
} PyArrayMethod_Context;
^
../numpy/_core/src/umath/loops.h.src:36:42: note: previous definition is here
typedef struct PyArrayMethod_Context_tag PyArrayMethod_Context;
```
The warning with Clang 14 is:
```
[3/4] Compiling C object numpy/_core/_multiarray_umath.cpython-39-darwin.so.p/meson-generated_einsum.c.o
../numpy/_core/src/multiarray/einsum.c.src:408:32: warning: unknown warning group '-Wmaybe-uninitialized', ignored [-Wunknown-warning-option]
^
[173/301] Compiling C object numpy/_core/_multiarray_umath.cpython-39-darwin.so.p/meson-generated_scalarmath.c.o
../numpy/_core/src/umath/scalarmath.c.src:1810:36: warning: unknown warning group '-Wmaybe-uninitialized', ignored [-Wunknown-warning-option]
#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
^
```
These pragmas are outdated and shouldn't be used. Instead, use
compiler flags (if really needed) that are feature-tested for actually
existing for the compiler that is used in the build.
These were yielding unused function warnings with Clang 14, and turned out to indeed be unused. The code was an exact copy of the same function in `loops_unary_complex_dispatch.c.src`, where it was used.
Needed to include the bug fix for using complex types with C11 (Cython PR 5809).
1960458 to
0ee0b6f
Compare
Action to drop the loops_minmax.dispatch.c.src change was taken
|
Thanks Ralf, looks all good now, let's get this in. |
I'm going to skip this. There are enough differences between 1.26.x and main that it feels risky. I plan on maybe two more 1.26 releases, and if we can get through those I'll be happy. |
|
Thanks for this, the warnings were indeed annoying! |
The large number of build warnings on macOS was getting annoying, so I dug in and solved all but a handful of them (it was 51, now there are 5 left). Those last ones were tricky to address, and silencing
-Wunused-functiondidn't seem quite right.One main cause of warnings was the use of C11 features. Rather than trying to change that code, it made sense to start requiring C11 explicitly. The biggest change in C11 was to make previously-mandatory C99 features like
complex.hand complex types optional - and we were already allowing that of course, because MSVC doesn't support complex types. Since all compilers we care about support C11, and thetypedefredefinitions that we use in multiple places are a C11-only feature, documenting that C11 is what we require see and compiling with-std=c11seems fine.This PR also cleans up uses of
#pragma GCC diagnostic. Those should be avoided, especially for-Wmaybe-uninitialized, because Clang also looks at these pragmas and emits warnings when it doesn't understand them (and-Wmaybe-uninitializeddoesn't exist there). There are usually better ways to deal with the problem, like changing the code that emits the warnings.Finally worth pointing out is one odd bug inloops_minmax.dispatch.c.srcwhich disabled rather than enabled some code that was meant to run on Arm only. This was clearly a typo; tests all seem to pass; I didn't investigate performance.