Skip to content
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.

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

MAINT: Fix clang build and remove some unicode characters #14023

Merged
merged 1 commit into from
May 12, 2021

Conversation

peterbell10
Copy link
Member

@peterbell10 peterbell10 commented May 11, 2021

When compiling with clang 10 or 11, I get errors in biasedurn/wnchyppr.cpp:

scipy/stats/biasedurn/wnchyppr.cpp:553:20: error: non-constant-expression cannot be narrowed from type 'int32_t' (aka 'int') to 'double' in initializer list [-Wc++11-narrowing]
   double xx[2] = {x, n-x};

On macOS, where clang is the default, this was fixed by adding a command line flag -Wno-narrowing. This PR takes the other approach of adding the appropriate static_casts.

Also, when editing this file I get invalid unicode character errors. So replaced those with plain ascii.

@rgommers rgommers added this to the 1.7.0 milestone May 11, 2021
@rgommers rgommers requested a review from mckib2 May 11, 2021 13:32
@rgommers rgommers added maintenance Items related to regular maintenance tasks scipy.stats labels May 11, 2021
Copy link
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 Peter. LGTM - the extra compile flags approach isn't portable indeed. I'll defer to @mckib2 for the rest.

@mckib2
Copy link
Contributor

mckib2 commented May 11, 2021

These source files are vendored from the BiasedUrn R package version that was approved for use under the BSD-new license. We added the compiler flags to avoid modifying any of the sources as much as possible, but if there are portability issues that can't be easily solved via flags during the build, then I don't have any reservations about making changes. We can let the git history speak for itself as far as tracking what modifications we've made. The changes to use static_cast is indeed the right thing to here

@tylerjereddy
Copy link
Contributor

Alright, two core dev approvals and CI fails don't look related.

@tylerjereddy tylerjereddy merged commit 0c8f0a8 into scipy:master May 12, 2021
@tylerjereddy
Copy link
Contributor

Thanks @peterbell10 et al.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks scipy.stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants