10000 MAINT: remove unused functions, rearrange headers (from CC=clang) by mattip · Pull Request #14534 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Sep 26, 2019
Merged

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

merged 3 commits into from
Sep 26, 2019

Conversation

mattip
Copy link
Member
@mattip mattip commented Sep 17, 2019

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
mattip commented Sep 17, 2019
View reviewed changes
numpy/core/src/umath/scalarmath.c.src


#if !defined(NPY_PY3K)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
mattip commented Sep 17, 2019
View reviewed changes
numpy/linalg/umath_linalg.c.src Outdated
#typ = DOUBLECOMPLEX_t#
#ftyp = fortran_doublecomplex#
#realtyp = double#
#lapack_func = zgeev#
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mattip
mattip commented Sep 17, 2019
View reviewed changes
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"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor
@zoj613 zoj613 Feb 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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:
8000
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
Copy link
Member Author
mattip commented Sep 18, 2019

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

@mattip
Copy link
Member Author
mattip commented Sep 18, 2019

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
seberg reviewed Sep 19, 2019
View reviewed changes
Copy link
Member
@seberg seberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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


#if !defined(NPY_PY3K)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

numpy/linalg/umath_linalg.c.src Outdated
@@ -1858,6 +1864,7 @@ typedef struct geev_params_struct {
char JOBVR;
} GEEV_PARAMS_t;

#if 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

numpy/linalg/umath_linalg.c.src Outdated
#typ = DOUBLECOMPLEX_t#
#ftyp = fortran_doublecomplex#
#realtyp = double#
#lapack_func = zgeev#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
eric-wieser reviewed Sep 19, 2019
View reviewed changes
numpy/linalg/umath_linalg.c.src Outdated
#typ = DOUBLECOMPLEX_t#
#ftyp = fortran_doublecomplex#
#realtyp = double#
#lapack_func = zgeev#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
eric-wieser reviewed Sep 19, 2019
View reviewed changes
numpy/linalg/umath_linalg.c.src Outdated
@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mattip
Copy link
Member Author
mattip commented Sep 19, 2019

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

@charris 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
Copy link
Member Author
mattip commented Sep 23, 2019

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

seberg
seberg approved these changes Sep 26, 2019
View reviewed changes
Copy link
Member
@seberg seberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

@mattip @seberg
Apply suggestions from code review
758b8bd 
Co-Authored-By: Sebastian Berg <sebastian@sipsolutions.net>
@seberg seberg merged commit 4b1cd4e into numpy:master Sep 26, 2019
@seberg 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 mattip mentioned this pull request Oct 7, 2019
Merged
@mattip mattip deleted the clang-cleanup branch June 8, 2020 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eric-wieser eric-wieser eric-wieser left review comments

@zoj613 zoj613 zoj613 left review comments

@seberg seberg seberg approved these changes

Assignees
No one assigned
Labels
component: build
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mattip @rgommers @seberg @eric-wieser @zoj613
0