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: Squelch parenthesis warnings from GCC #8655

Merged
merged 1 commit into from
Feb 22, 2017

Conversation

eric-wieser
Copy link
Member

These were previously avoided by patching f2c, but this was more work for
little gain, and the patch was not committed.

it's readable, and we know what is written is correct. So don't warn about
them.
*/
#pragma GCC diagnostic ignored "-Wparentheses"
Copy link
Member

Choose a reason for hiding this comment

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

I suspect you want to make this depend on gcc, e.g.,

#if defined(__GNUC__)
#pragma GCC diagnostic ignored "-Wparentheses"
#endif

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that seems sane. Is that the right macro to switch on?

Copy link
Member Author

Choose a reason for hiding this comment

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

These were previously avoided by patching f2c, but this was more work for
little gain, and the patch was not committed.
@eric-wieser
Copy link
Member Author

@charris: done

@mhvk
Copy link
Contributor

mhvk commented Feb 22, 2017

This all looks OK, but wouldn't it make more sense to remove the compiler option in setup.py? Although a quick look at this led me to a numpy/distutils/system_info which does not seem exactly easy to parse... (fairly easy to see how to add compiler options, bit less obvious how to remove them). Anyway, only if this somehow rings a bell of, yes, of course, this is where it should be done...

@eric-wieser
Copy link
Member Author

eric-wieser commented Feb 22, 2017

@mhvk: Argument against that would be that some files do want to be compiled with that warning (admittedly just python_xerbla.c, I think`). Perhaps the warnings can be assigned on a per-file basis in systeminfo, but I have no idea about how to use that.

Either way, it seems neater to pair the warning suppression with the faulty generator that causes those warnings

@charris
Copy link
Member

charris commented Feb 22, 2017

The warning is enabled by -Wall, there is probably a flag to turn it off, -Wno-parentheses maybe, but I'm not sure how to set the flags for individual files and we would like it for general files. Apparently clang doesn't warn for the operator precendence case, or at least didn't, so we probably don't need to worry about that.

@@ -16,6 +16,15 @@ extern doublereal dlamch_(char *);

extern doublereal dlapy2_(doublereal *x, doublereal *y);

/*
Copy link
Member

Choose a reason for hiding this comment

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

Multiline comments like

/*
 * blah
 * blah
 */

usw.

Copy link
Member Author

@eric-wieser eric-wieser Feb 22, 2017

Choose a reason for hiding this comment

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

Note that right now all the generated code goes through a comment transformation script (starting from /* ... */ at the ends of each line) that makes them look like the one I wrote.

Internal-to-lapack_lite consistency seems more valuable here (see also: python_xerbla.c)

@charris
Copy link
Member

charris commented Feb 22, 2017

We do have a style guide, doc/C_STYLE_GUIDE.rst.txt ;)

@charris
Copy link
Member

charris commented Feb 22, 2017

Oh, well, it was worth a try.

@charris charris merged commit a800659 into numpy:master Feb 22, 2017
@charris
Copy link
Member

charris commented Feb 22, 2017

Thanks Eric.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants