MAINT: remove unused functions, rearrange headers (from CC=clang) #14534

mattip · 2019-09-17T20:32:20Z

Cleanup unused function and tweak order of header include to get a clean build on clang (at least on my machine). I built this on top of #14527 to turn all warnings into errors after the build_src configuration step.

mattip · 2019-09-17T20:35:53Z

numpy/core/src/umath/scalarmath.c.src

+
+
+#if !defined(NPY_PY3K)
+


This splits the @name@_@which@ templates into two parts: one for python2 using long (below) and one for both 2 and 3 using float (above).

Do we want to backport this, or shouldn't we just ignore python 2 for such a specific, rarely touched, code chunk?

No need to backport. NEP 14 is not clear on this, but I think we have not been actively removing python2 code from the master branch yet.

mattip · 2019-09-17T20:38:21Z

numpy/linalg/umath_linalg.c.src

+   #typ = DOUBLECOMPLEX_t#
+   #ftyp = fortran_doublecomplex#
+   #realtyp = double#
+   #lapack_func = zgeev#


It turns out the cfloat loops were not used. The python function upcasts to cdouble. I am not sure why they were added in on commit in gh-3220, but then neutralized in another.

Oh, interesting. Looking yet another time, the python code specifically uses double precision always, but then casts back to single precision. Is there something specific about eig that makes single precision numerically bad? Otherwise it seems like maybe we should register all loops and remove forcing the signature to double.

From what I've seen, the other gufuncs register all the loops. and the deliberately avoid using them in python. Can we do the same thing here?

removed this from the PR, will reintroduce a cleaner solution in a future one

mattip · 2019-09-17T20:41:54Z

numpy/random/src/distributions/distributions.h

@@ -1,13 +1,12 @@
 #ifndef _RANDOMDGEN__DISTRIBUTIONS_H_
 #define _RANDOMDGEN__DISTRIBUTIONS_H_

-#pragma once
+#include "Python.h"
+#include "numpy/npy_common.h"


this change and the others in the random/src files were needed to get rid of a warning. Googling it seems to suggest there is a circular include graph. This reordering solved it and seems harmless.

In file included from /usr/include/python3.6m/Python.h:77: /usr/include/python3.6m/pytime.h:146:56: error: declaration of 'struct timespec' will not be visible outside of this function [-Werror,-Wvisibility] PyAPI_FUNC(int) _PyTime_AsTimespec(_PyTime_t t, struct timespec *ts);

I keep getting this warning anytime I compile C code that includes <numpy/random/*.h> with -std=c99. It is annoying and hides actual informative warnings. It will fill up the screen if included in multiple modules. Any ideas how I can get rid of these without disabling the visibility warning? @mattip

Please open a new issue and provide information so we can reproduce: what compiler and platform are you using? What version of NumPy? Some sample code and the actual warning you get would also help.

Okay, I will try and see if I can reproduce with an example. The warning is exactly the same as the one you mentioned above:

In file included from /usr/include/python3.6m/Python.h:77: /usr/include/python3.6m/pytime.h:146:56: error: declaration of 'struct timespec' will not be visible outside of this function [-Werror,-Wvisibility] PyAPI_FUNC(int) _PyTime_AsTimespec(_PyTime_t t, struct timespec *ts);

It pops up anytime I compiled C code with numpy/npy_common.h and numpy/random/distributions.h. It is caused by the Python.h header included in those files. I can't silence it. Im on Manjaro linux 64bit, python 3.8/3.9.

rgommers · 2019-09-18T04:00:02Z

LGTM. Still seeing some warnings but definitely cleaner. Leaving out the ones from _configtest, here's what's left for me:

In file included from numpy/core/src/npysort/selection.c.src:22:
numpy/core/src/common/npy_partition.h.src:98: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) {
        ~~~~~ ^  ~~~~~~~~~~~~
numpy/core/src/common/npy_partition.h.src:116: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) {
        ~~~~~ ^  ~~~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
numpy/core/src/npysort/selection.c.src:328:9: warning: code will never be executed [-Wunreachable-code]
        npy_intp k;
        ^~~~~~~~~~~
numpy/core/src/npysort/selection.c.src:326:14: note: silence by adding parentheses to mark code as explicitly
      dead
    else if (0 && kth == num - 1) {
             ^
             /* DISABLES CODE */ ( )
In file included from numpy/core/src/npysort/binsearch.c.src:6:
numpy/core/src/common/npy_binsearch.h.src:110:14: warning: result of comparison of constant 2 with expression
      of type 'NPY_SEARCHSIDE' is always false [-Wtautological-constant-out-of-range-compare]
    if (side >= NPY_NSEARCHSIDES) {
        ~~~~ ^  ~~~~~~~~~~~~~~~~
numpy/core/src/common/npy_binsearch.h.src:110:14: warning: result of comparison of constant 2 with expression
      of type 'NPY_SEARCHSIDE' is always false [-Wtautological-constant-out-of-range-compare]
    if (side >= NPY_NSEARCHSIDES) {
        ~~~~ ^  ~~~~~~~~~~~~~~~~
2 warnings generated.
24 warnings generated.
In file included from numpy/core/src/multiarray/item_selection.c:26:
numpy/core/src/common/npy_partition.h.src:98: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) {
        ~~~~~ ^  ~~~~~~~~~~~~
numpy/core/src/common/npy_partition.h.src:116: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) {
        ~~~~~ ^  ~~~~~~~~~~~~
In file included from numpy/core/src/multiarray/item_selection.c:27:
numpy/core/src/common/npy_binsearch.h.src:110:14: warning: result of comparison of constant 2 with expression
      of type 'NPY_SEARCHSIDE' is always false [-Wtautological-constant-out-of-range-compare]
    if (side >= NPY_NSEARCHSIDES) {
        ~~~~ ^  ~~~~~~~~~~~~~~~~
numpy/core/src/common/npy_binsearch.h.src:110:14: warning: result of comparison of constant 2 with expression
      of type 'NPY_SEARCHSIDE' is always false [-Wtautological-constant-out-of-range-compare]
    if (side >= NPY_NSEARCHSIDES) {
        ~~~~ ^  ~~~~~~~~~~~~~~~~
numpy/core/src/multiarray/item_selection.c:1249:28: warning: result of comparison of constant 1 with
      expression of type 'NPY_SELECTKIND' is always false [-Wtautological-constant-out-of-range-compare]
    if (which < 0 || which >= NPY_NSELECTS) {
                     ~~~~~ ^  ~~~~~~~~~~~~
numpy/core/src/multiarray/item_selection.c:1339:28: warning: result of comparison of constant 1 with
      expression of type 'NPY_SELECTKIND' is always false [-Wtautological-constant-out-of-range-compare]
    if (which < 0 || which >= NPY_NSELECTS) {
                     ~~~~~ ^  ~~~~~~~~~~~~
6 warnings generated.
numpy/core/src/umath/loops.c.src:2588:22: warning: code will never be executed [-Wunreachable-code]
        npy_intp n = dimensions[0];
                     ^~~~~~~~~~
numpy/core/src/umath/loops.c.src:2587:29: note: silence by adding parentheses to mark code as explicitly dead
    if (IS_BINARY_REDUCE && 0) {
                            ^
                            /* DISABLES CODE */ ( )
numpy/core/src/umath/loops.c.src:2588:22: warning: code will never be executed [-Wunreachable-code]
        npy_intp n = dimensions[0];
                     ^~~~~~~~~~
numpy/core/src/umath/loops.c.src:2587:29: note: silence by adding parentheses to mark code as explicitly dead
    if (IS_BINARY_REDUCE && 0) {
                            ^
                            /* DISABLES CODE */ ( )
numpy/core/src/umath/loops.c.src:2588:22: warning: code will never be executed [-Wunreachable-code]
        npy_intp n = dimensions[0];
                     ^~~~~~~~~~
numpy/core/src/umath/loops.c.src:2587:29: note: silence by adding parentheses to mark code as explicitly dead
    if (IS_BINARY_REDUCE && 0) {
                            ^
                            /* DISABLES CODE */ ( )

That's with

$ clang --version
Apple LLVM version 10.0.1 (clang-1001.0.46.4)
Target: x86_64-apple-darwin18.7.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

mattip · 2019-09-18T06:33:31Z

I am not seeing them on clang 8, and it doesn't seem like I can easily get clang 10 on ubuntu.

mattip · 2019-09-18T06:54:15Z

We may need to discuss some of the additional warnings further. For instance, the first one marks the check that which is in-bounds as unnecessary since which is an enum. However this function is called from a NumPy C-API function with the signature

PyArray_Partition(PyArrayObject *op, PyArrayObject * ktharray, int axis,
                  NPY_SELECTKIND which)

which is exported as a C function from the _multiarray_umath shared object. That means the which parameter in the function is actually mislabelled, the exported function can be called with whatever kind of int the compiler creates for an enum, and I think the correct fix for this warning is to change the C signature of the PyArray_Partition function (and check the validity of the which parameter there). Changing the signature of a NumPy C-API function is beyond this PR.

seberg

LGTM, only real comment is that it would be nice to know whether simply enabling the single precision loops is not the better solution. I understand that that is not an easy thing to say unless you happen to know. @pv if you have a minute. Do you know whether np.linalg.eig has a good reason to force calculation in double precision?

seberg · 2019-09-19T02:15:34Z

numpy/core/src/umath/scalarmath.c.src

+
+
+#if !defined(NPY_PY3K)
+


Do we want to backport this, or shouldn't we just ignore python 2 for such a specific, rarely touched, code chunk?

seberg · 2019-09-19T02:16:30Z

numpy/linalg/umath_linalg.c.src

@@ -1858,6 +1864,7 @@ typedef struct geev_params_struct {
    char JOBVR;
 } GEEV_PARAMS_t;

+#if 0


I guess these are obviously just there to help debugging. Maybe create a local #define, to make it a bit more obvious (and easier to toggle both)?

seberg · 2019-09-19T02:22:24Z

numpy/linalg/umath_linalg.c.src

+   #typ = DOUBLECOMPLEX_t#
+   #ftyp = fortran_doublecomplex#
+   #realtyp = double#
+   #lapack_func = zgeev#


Oh, interesting. Looking yet another time, the python code specifically uses double precision always, but then casts back to single precision. Is there something specific about eig that makes single precision numerically bad? Otherwise it seems like maybe we should register all loops and remove forcing the signature to double.

eric-wieser · 2019-09-19T05:07:54Z

numpy/linalg/umath_linalg.c.src

+   #typ = DOUBLECOMPLEX_t#
+   #ftyp = fortran_doublecomplex#
+   #realtyp = double#
+   #lapack_func = zgeev#


Instead of not generating this code at all, can we generate it inside #if 0? Writing these templates can be hard to get write, so it seems a shame to back out the partial work done for floats.

eric-wieser · 2019-09-19T05:08:41Z

numpy/linalg/umath_linalg.c.src

@@ -861,6 +865,7 @@ nan_@TYPE@_matrix(void *dst_in, const LINEARIZE_DATA_t* data)
    }
 }

+#if 0
 static NPY_INLINE void
 zero_@TYPE@_matrix(void *dst_in, const LINEARIZE_DATA_t* data)


Could we put __attribute__((unused)) on these instead? #if 0 code is in danger of compilation bitrot.

does msvc have an attribute((unused)) equivalent?

mattip · 2019-09-19T21:02:58Z

Will wait for gh-14527 and gh-14518 to make running this easier

mattip · 2019-09-23T17:47:20Z

I removed numpy/linalg/umath_linalg.c.src from this PR since it was a bit more tricky than just removing/rearranging code.

seberg

Ah, I think on my first read I missed that this was about nb_slots and the long one does not exist on Py3. Two tiny requests...

numpy/core/src/umath/scalarmath.c.src

Co-Authored-By: Sebastian Berg <sebastian@sipsolutions.net>

mattip added the component: build label Sep 17, 2019

mattip commented Sep 17, 2019

View reviewed changes

seberg reviewed Sep 19, 2019

View reviewed changes

eric-wieser reviewed Sep 19, 2019

View reviewed changes

charris changed the title ~~BUILD: remove unused functions, rearrange headers (from CC=clang)~~ BLD: remove unused functions, rearrange headers (from CC=clang) Sep 20, 2019

mattip added 2 commits September 20, 2019 17:55

BUILD: remove unused functions, rearrange headers (from CC=clang)

f41d704

MAINT: check enum at API call, not in provate function

26079cf

mattip force-pushed the clang-cleanup branch from b5dac79 to 26079cf Compare September 20, 2019 14:58

seberg approved these changes Sep 26, 2019

View reviewed changes

Apply suggestions from code review

758b8bd

Co-Authored-By: Sebastian Berg <sebastian@sipsolutions.net>

seberg merged commit 4b1cd4e into numpy:master Sep 26, 2019

seberg changed the title ~~BLD: remove unused functions, rearrange headers (from CC=clang)~~ MAINT: remove unused functions, rearrange headers (from CC=clang) Sep 26, 2019

mattip mentioned this pull request Oct 7, 2019

BUILD: change to build_src --verbose-cfg, runtests.py --debug-info #14630

Merged

mattip deleted the clang-cleanup branch June 8, 2020 06:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAINT: remove unused functions, rearrange headers (from CC=clang) #14534

MAINT: remove unused functions, rearrange headers (from CC=clang) #14534

mattip commented Sep 17, 2019

mattip Sep 17, 2019

seberg Sep 19, 2019

mattip Sep 19, 2019

mattip Sep 17, 2019

seberg Sep 19, 2019

eric-wieser Sep 19, 2019

mattip Sep 20, 2019

mattip Sep 17, 2019

zoj613 Feb 24, 2021

mattip Feb 24, 2021

zoj613 Feb 24, 2021 •

edited

Loading

rgommers commented Sep 18, 2019

mattip commented Sep 18, 2019

mattip commented Sep 18, 2019

seberg left a comment

seberg Sep 19, 2019

seberg Sep 19, 2019

seberg Sep 19, 2019

eric-wieser Sep 19, 2019

eric-wieser Sep 19, 2019

mattip Sep 19, 2019

mattip commented Sep 19, 2019

mattip commented Sep 23, 2019

seberg left a comment



		#if !defined(NPY_PY3K)



		#if !defined(NPY_PY3K)

MAINT: remove unused functions, rearrange headers (from CC=clang) #14534

MAINT: remove unused functions, rearrange headers (from CC=clang) #14534

Conversation

mattip commented Sep 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoj613 Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

rgommers commented Sep 18, 2019

mattip commented Sep 18, 2019

mattip commented Sep 18, 2019

seberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattip commented Sep 19, 2019

mattip commented Sep 23, 2019

seberg left a comment

Choose a reason for hiding this comment

zoj613 Feb 24, 2021 •

edited

Loading