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

Inconsistent warning flags specified in CMakeLists.txt for different compilers (possibly by mistake) #928

Closed
oddlama opened this issue Nov 5, 2018 · 4 comments

Comments

@oddlama
Copy link

oddlama commented Nov 5, 2018

While skimming through the CMakeLists.txt, I found that several warning flags have inconsistent (opposite) values for gcc and clang, which seems to be a mistake (please elaborate if I'm wrong).

First inconsistency -Wzero-as-null-pointer-constant

For gcc, -Wzero-as-null-pointer-constant is explicitly enabled if the host uses gcc >= 5.0,
while for clang -Wno-zero-as-null-pointer-constant (Note the -Wno-) is set and therefore the warning is disabled.

Second inconsistency -Wshadow

Also in gcc's PEDANTIC_COMPILE_FLAGS, -Wshadow is followed by -Wno-shadow:

set(PEDANTIC_COMPILE_FLAGS -pedantic-errors -Wall -Wextra -pedantic
    -Wold-style-cast -Wundef
    -Wredundant-decls -Wshadow [...] -Wno-shadow)

Other issues

Possibly there are other inconsistencies or mistakes, but as the authors intent does not appear clearly to me, I am unable to further judge the other flags.
I just wanted to point out, that almost every flag passed to clang is prefixed with -Wno-, and as -Wno-zero-as-null-pointer-constant appears to be a mistake, it might be worth having a look at the other flags, too.

@eliaskosunen
Copy link
Contributor

eliaskosunen commented Nov 5, 2018

I was the one that wrote those flags, see #736.

See git blame for CMakeLists.txt for the commit that introduced these changes. You can see, that gcc follows the scheme of enabling every flag possible, while clang uses -Weverything and then disables some of the noisier warnings.

Point 1:
I honestly don't remember what was my reasoning behind this, but it looks like a honest mistake. That warning should be enabled on both gcc and clang, and I can't think of any reason to disable it on clang, given that it's also enabled on gcc.

Point 2:
This was changed later here, I don't know why the change was done nor why it was done this way instead of simply removing the flag.

@oddlama
Copy link
Author

oddlama commented Nov 6, 2018

Fair enough. Although I cannot find that -Weverything is being used here, only -Wall for both compilers.

@eliaskosunen
Copy link
Contributor

See the blame from my previous comment. Line 96 has -Weverything. This was rather sloppily changed in 50b18a, without removing the -Wnos.

@vitaut
Copy link
Contributor

vitaut commented Nov 7, 2018

Looks like I accidentally messed up the warnings. Should be cleaned in 34030de.

@vitaut vitaut closed this as completed Nov 7, 2018
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

3 participants