Skip to content

cmake-js disables all compiler warnings on OSX #132

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

Closed
corystegel opened this issue Jul 13, 2018 · 3 comments
Closed

cmake-js disables all compiler warnings on OSX #132

corystegel opened this issue Jul 13, 2018 · 3 comments

Comments

@corystegel
Copy link
Contributor

cmake-js unconditionally disables all compiler warnings on OSX by adding "-w" to the C++ compiler flags on OSX.

This should not be done because forcing all projects on OSX to hide all their compiler warnings is a very bad idea. Warnings should be controlled by the underlying CMakeLists.txt, not cmake-js.

@unbornchikken
Copy link
Member

There was a reason for this option, but I cannot remember exactly. I believe this originated from the corresponding node-gyp defaults. Anyway that would be a breaking change for already existing modules, they should add -w to their CMakeLists.txt once it get landed. Let me think about it.

@corystegel
Copy link
Contributor Author

corystegel commented Jul 16, 2018

To avoid breaking existing modules that have -Werror in their cmake perhaps add an option to cmake-js to disable all warnings which defaults to true? This would allow projects on OSX to enable warnings which isn't possible right now.

If "-w" is removed and a project is not escalating warnings to errors then all this will do is cause extra warning output to print to the console when the module is compiling which shouldn't break anything. It also lets developers know that areas of their code need attention.

If "-w" is removed and a project is escalating warnings to errors they are probably wrongly assuming that they have no warnings. This would cause these existing modules to no longer compile if they actually did have warnings that were previously being hidden.

@unbornchikken
Copy link
Member

Fixed as of v4.0.0, see changelog.

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

No branches or pull requests

2 participants