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

STY: reduce number of C compiler warnings #3537

Merged
merged 1 commit into from
Apr 27, 2014
Merged

Conversation

larsmans
Copy link
Contributor

Mostly dead code, unused variables, non-prototype (K&R) declarations. The special/cephes dir in particular contains some very old-fashioned C code. (It also carries no license...)

Reduces warning log by exactly 300 lines on my box, or ~5%.

@rgommers rgommers added the PR label Apr 10, 2014
@argriffing
Copy link
Contributor

The PR looks OK to me because the changes in fftpack and quadpack seem to be only in the python interfaces, and I think scipy has effectively forked cephes, so changes within this cephes code itself (not just interface) should be OK too.

@rgommers
Copy link
Member

@larsmans thanks, this kind of PR is quite useful imho.

@larsmans
Copy link
Contributor Author

I didn't touch any of the Fortran code because I'm not familiar with that language, but gfortran spews out endless warnings as well -- maybe some can have a look at that.

@@ -482,7 +482,7 @@ double *loss; /* estimates loss of significance */
a = f;
}

ia = round(a);
round(a); /* XXX result not used, why? */
Copy link
Member

Choose a reason for hiding this comment

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

This should be resolved before merging. I suspect the line can be deleted. @pv?

Copy link
Member

Choose a reason for hiding this comment

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

The ia = ...; ib = ... is just copypaste from other routines.
The statement should be removed completely if unused.

@WarrenWeckesser
Copy link
Member

Thanks for working on this! 5% fewer warnings is a great start.

I don't think the change in special/cephes/euclid.c is necessary, because the entire file is not used. I just built scipy with the file removed, and had no problem with the build or the tests.

@larsmans
Copy link
Contributor Author

@WarrenWeckesser I was wondering what that euclid.c file was doing at all but I didn't fully grasp the macroized build system for cephes. Should I just remove it in this PR? I found some more K&R definitions that can be changed to prototypes.

@rgommers
Copy link
Member

The question is if we want to bother hooking up euclid as special.gcd. It's basically a more efficient version of https://docs.python.org/3/library/fractions.html#fractions.gcd (which is dead simple). If not, we can remove the file.

@@ -61,6 +61,7 @@

int merror = 0;

#if 0 /* unused */
Copy link
Member

Choose a reason for hiding this comment

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

Go ahead and delete it. Having #if 0 stuff around is not good.

@larsmans
Copy link
Contributor Author

Ok, pushed a new version. Also rebased on current master.

Removed the Cephes rational arithmetic functions (euclid.c), which were
entirely unused.

Also dead statements, non-prototype (K&R) declarations.

Reduces warning log by exactly 300 lines on my box.
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c1e9d1d on larsmans:warnings into * on scipy:master*.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c1e9d1d on larsmans:warnings into e9dadc9 on scipy:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c1e9d1d on larsmans:warnings into e9dadc9 on scipy:master.

@WarrenWeckesser
Copy link
Member

👍

pv added a commit that referenced this pull request Apr 27, 2014
STY: reduce number of C compiler warnings
@pv pv merged commit 40f6ac1 into scipy:master Apr 27, 2014
@pv
Copy link
Member

pv commented Apr 27, 2014

LGTM, merged

@pv pv added this to the 0.15.0 milestone Apr 27, 2014
@larsmans larsmans deleted the warnings branch April 27, 2014 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants