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

Fix compiler warnings #1725

Merged
merged 1 commit into from Feb 20, 2013
Merged

Conversation

dmcdougall
Copy link
Member

Some of the compiler warnings started to bug me, so I tried to fix them. They're usually harmless, but sometimes they are not, as I will explain.

Currently, these are the warnings that are thrown during a python2.7 build of matplotlib (clang on a mac is used here)

After this patch is applied, these are the warnings that are still left. The first four are due to a 'bug' in the python API; treating string literals as char * rather than const char *. There is a patch upstream for it, but I'm not sure if it will be back-ported to python 2.7.4.

The last bunch of Tcl/Tk warnings are because the compiler is being handed both a -c flag and a -framework T{cl,k} flag, which are meant solely for the compilation and linking steps, respectively. To fix them, one would need to separate out the compile and link steps, but I don't know how to do that with distutils, and they only appear on the mac. Linux and windows compilers do not support the -framework flag.

I have tested this patch and I get a successful build with both Apple's builds of gcc and clang. All the tests pass locally with python 2.7. It would be nice to have these tested with python3, and a linux flavour of compiler.

I also fixed a typo in a comment.

@dmcdougall
Copy link
Member Author

I didn't target 1.2.x on purpose. These don't fix bugs, and I didn't want them in 1.2.x 'just in case' they broke something I didn't think about.

npy_PyFile_DupClose(py_file, fp);
if (npy_PyFile_DupClose(py_file, fp)) {
throw Py::RuntimeError("Error closing dupe file handle");
}
Copy link
Member Author

Choose a reason for hiding this comment

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

All this block does is make use of the value npy_PyFile_DupClose returns.

@tacaswell
Copy link
Member

debian sid, gcc 4.7.2, python 2.7.3

warnings on master https://gist.github.com/4683894

Warnings on this branch https://gist.github.com/4683594

// matplotlib. This is here so that the compiler doesn't complain
// that it is unused.
extern "C" {
__attribute__((unused)) static void simple_capsule_dtor(void *ptr);
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I have absolutely no idea what this does (black magic 😄). Could somebody who does give it the 👍 please?

Copy link
Member

Choose a reason for hiding this comment

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

Seems fine, except I wish there were a cleaner way to hide this warning... Don't know what that would be.

@pelson
Copy link
Member

pelson commented Feb 4, 2013

Other than my lack of understanding which I have pointed out, I'd be happy enough to merge this. Your right about targetting v1.3.x - I would be very uncomfortable with this kind of change slipping into v1.2.x as we could easily be introducing a subtle regression...

@mdboom
Copy link
Member

mdboom commented Feb 4, 2013

Looks fine -- but I may have some further changes shortly for some more warnings I'm still getting on my platform.

@dmcdougall
Copy link
Member Author

@mdboom Go for it! :)

@mdboom
Copy link
Member

mdboom commented Feb 20, 2013

I'm not seeing the compiler warnings I was seeing before -- perhaps I hadn't enough coffee that day. I think this is good to merge...

mdboom added a commit that referenced this pull request Feb 20, 2013
@mdboom mdboom merged commit 091c149 into matplotlib:master Feb 20, 2013
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

4 participants