Skip to content

MAINT, BLD: Fix unused inline functions warnings on clang#25555

Merged
rgommers merged 2 commits intonumpy:mainfrom
seiko2plus:issue_25471
Jan 10, 2024
Merged

MAINT, BLD: Fix unused inline functions warnings on clang#25555
rgommers merged 2 commits intonumpy:mainfrom
seiko2plus:issue_25471

Conversation

@seiko2plus
Copy link
Copy Markdown
Member

@seiko2plus seiko2plus commented Jan 9, 2024

closes #25471

@seiko2plus seiko2plus marked this pull request as ready for review January 9, 2024 14:05
@seiko2plus seiko2plus added the 09 - Backport-Candidate PRs tagged should be backported label Jan 9, 2024
Mousius
Mousius approved these changes Jan 9, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@Mousius Mousius left a comment

Choose a reason for hiding this comment

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

Good fixing @seiko2plus 😸

@ngoldbaum
Copy link
Copy Markdown
Member

ngoldbaum commented Jan 9, 2024 •
edited
Loading

Thanks for fixing this! It makes it harder to see when I add compiler warnings if there is noise every time.

I still see one unused function warning compiling this PR on an arm64 mac:

../numpy/linalg/umath_linalg.cpp:2303:1: warning: unused function 'call_geev' [-Wunused-function]
call_geev(GEEV_PARAMS_t<fortran_complex>* params)
^
1 warning generated.

@seiko2plus
MAINT: Comment in the unused overload of call_geev
728f550 
  To avoid unsed warrning on clang
@seiko2plus
Copy link
Copy Markdown
Member Author

seiko2plus commented Jan 9, 2024

call_geev(GEEV_PARAMS_t<fortran_complex>* params)

It seems this overloaded function is defined but has never been used or disabled.

@seiko2plus
Copy link
Copy Markdown
Member Author

seiko2plus commented Jan 9, 2024

close to rerun CI jobs

@seiko2plus seiko2plus closed this Jan 9, 2024
@seiko2plus seiko2plus reopened this Jan 9, 2024
rgommers
rgommers approved these changes Jan 10, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks Sayed, this LGTM and fixes all the build warnings on macOS with the current Clang version.

Before merging, it'd be good to update the description for the sin/cos disabling, as I commented on inline.

The Azure timeouts can be ignored. I re-ran the two jobs, but they're hanging for >30 minutes already on:

Installing rtools...

numpy/_core/src/umath/loops_trigonometric.dispatch.c.src
/* Disable SIMD code and revert to libm: see
* https://mail.python.org/archives/list/numpy-discussion@python.org/thread/C6EYZZSR4EWGVKHAZXLE7IBILRMNVK7L/
* for detailed discussion on this*/
#if 0 // NPY_SIMD_F64
Copy link
Copy Markdown
Member

@rgommers rgommers Jan 10, 2024

Choose a reason for hiding this comment

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

This is a functional change, right, not just fixing a build warning - the disabling wasn't done earlier after the May'23 mailing list discussion? If so, it'd be good to add this to the PR title and description.

Copy link
Copy Markdown
Member

@Mousius Mousius Jan 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Could we just merge this and do a follow up to remove the F64 code as a distinct change? (the git history would reflect that discussion rather than being tied to this one) It feels like we're just cluttering the code base with it at this point given it'll need changing to 1ULP or adding the accuracy toggle.

Copy link
Copy Markdown
Member

@Mousius Mousius Jan 10, 2024

Choose a reason for hiding this comment

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

Also, not a functional change, it was disabled here first:

numpy/numpy/_core/src/umath/loops_trigonometric.dispatch.c.src

Lines 391 to 395 in 6e3b923

/* Disable SIMD code and revert to libm: see
* https://mail.python.org/archives/list/numpy-discussion@python.org/thread/C6EYZZSR4EWGVKHAZXLE7IBILRMNVK7L/
* for detailed discussion on this*/
//#if NPY_SIMD_F64 && NPY_SIMD_FMA3
#if 0

This function was just left out, causing the unused warning.

Copy link
Copy Markdown
Member

@rgommers rgommers Jan 10, 2024

Choose a reason for hiding this comment

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

Okay, thanks for checking! Wasn't clear to me from only the diff. Then I agree, let's get this in as is.

And +1 for figuring out whether or not to add the accuracy toggle, and otherwise removing the code.

@rgommers rgommers merged commit aeeb7f5 into numpy:main Jan 10, 2024
@rgommers rgommers added this to the 2.0.0 release milestone Jan 10, 2024
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rgommers rgommers rgommers approved these changes

@Mousius Mousius Mousius approved these changes

Assignees

No one assigned

Labels

03 - Maintenance

Projects

None yet

Milestone

2.0.0 release

Development

Successfully merging this pull request may close these issues.

BLD: unused function warnings in current build

5 participants

@seiko2plus @ngoldbaum @rgommers @Mousius @charris