Default -Wall -Werror and the skipped errors

[mgeplf]

@wizmer copied the flags from:
https://github.com/BlueBrain/hpc-coding-conventions/blob/master/cpp/cmake/bob.cmake#L192

What is the plan for choosing what is on that list and what is not?

As the compilers have become more feature-ful, and more aggressive with optimizations, my defensive programming sensibilities have kicked in even stronger. Turning on -Wall & -Werror seems like a necessity to me (with the caveat that one can choose to only enable them for non-Release builds - but the CI should run those for testing).

With that in mind, I like to try and keep the list of exceptions minimal. On that list, for instance, I think -Wswitch-enum is an important warning: the fact that one has to make some switch statements have to be complete is outweighed by missing cases.

[tristan0x]

Thank you for your interest @mgeplf

The flags @wizmer found is based on the coding conventions https://github.com/BlueBrain/hpc-coding-conventions/blob/master/cpp/Tooling.md

-Wswitch-enum seems an interesting flag we can add. Which compilers/versions support it?

[mgeplf]

I think the sentiment, on that page:
Clang: -Werror -Weverything. And then disable those you dislike. is at play here - I just wouldn't disable that warning, like is done here:
https://github.com/BlueBrain/hpc-coding-conventions/blob/master/cpp/cmake/bob.cmake#L203

I guess I'm trying to see if we can build a consensus about what which ones can be disabled.

For instance, -Wno-padded makes perfect sense to disable globally (I see it as useful to turn on once and a while to see good/bad the memory layout is), but (maybe?) the -Wno-documentation should be up to the project maintainer (even though many/most would disable it?).

BTW, to answer Which compilers/versions support it? It seems that GCC 4.8 does, and at least clang 4, so it's been around a while. How aggressive it is might have changed over time, as these things do.

[tristan0x]

I think the sentiment, on that page:
Clang: -Werror -Weverything. And then disable those you dislike. is at play here

I agree we have to rephrase this.

I just wouldn't disable that warning, like is done here:
https://github.com/BlueBrain/hpc-coding-conventions/blob/master/cpp/cmake/bob.cmake#L203

You are right, I updated it accordingly in #34

but (maybe?) the -Wno-documentation should be up to the project maintainer (even though many/most would disable it?).

I was a bit heavy-handed with -Wno-documentation. This is -Wno-documentation-unknown-command I really needed. I removed it also.

Besides, bob.cmake is not yet supported. It is WIP, and thank you for improving it.

[mgeplf]

So the plan is to configure the project w/ bob, and then have user modify the CMAKE_CXX_FLAGS manually?

I was hoping to open discussion on what a 'good' subset of -Wno-* because I don't have a feeling what should and should not be recommended (been a long time since I've been in C++ land.) For instance, no-global-constructors and no-exit-time-destructors basically preclude having statics - something that is good in theory, but in practice I think it will basically be disabled in (almost?) all cases.

We can take the discussion to your pull request, if it's easier. Or, just go w/ #34's advice. At that point, I would make it clear in the doc what the best practice is for modifying flags (one of CMake's weaknesses, imo, but maybe there's a modern way to do it that I don't know about)

[pramodk]

@tristan0x : I think we should discuss if cmake.bob should be put in hpc coding convention. When I looked at that in zee first time, I couldn't find that easy / transparent (have to look at more details myself).

@matz-e and @ohm314 had similar impression but they could chime in with details.

[tristan0x]

@tristan0x : I think we should discuss if cmake.bob should be put in hpc coding convention. When I looked at that in zee first time, I couldn't find that easy / transparent (have to look at more details myself).

@matz-e and @ohm314 had similar impression but they could chime in with details.

I prefer to make the improve bob.cmake to make it easy to use rather than removing it. Besides, you are free not to use it.

[tristan0x]

So the plan is to configure the project w/ bob, and then have user modify the CMAKE_CXX_FLAGS manually?

This would be one solution. Or maybe something different, more integrated with CMake CMAKE_BUILD_TYPE and CTest submission modes:
Experimental, Nightly, and Continuous.

bob.cmake is currently not compatible with CMAKE_BUILD_TYPE because it conflicts with the flags set by CMake according to the value of this variable. bob does a lot of things. I use it for my projects but I understand it is an impacting change for others. For instance, right now bob raises an error if CMAKE_BUILD_TYPE is set, but maybe it could clear the CMAKE_CXX_FLAGS_${CMAKE_BUILD_TYPE} and CMAKE_C_FLAGS_${CMAKE_BUILD_TYPE} variables instead of complaining.

I was hoping to open discussion on what a 'good' subset of -Wno-* because I don't have a feeling what should and should not be recommended (been a long time since I've been in C++ land.) For instance, no-global-constructors and no-exit-time-destructors basically preclude having statics - something that is good in theory, but in practice I think it will basically be disabled in (almost?) all cases.

So far, I have never seen a Clang -Wno-X flag that should be set globally but the one that remains in #34. For instance, regarding no-global-constructors and no-exit-time-destructors you mentioned, I think we should explain in the guidelines why it is good in theory to preclude having statics without POD. Thus every project could still decide to avoid the warning but in full knowledge of the facts.

[pramodk]

I prefer to make the improve bob.cmake to make it easy to use rather than removing it. Besides, you are free not to use it.

Agree with improving aspects.

I don't want to judge it (as I haven't looked at details) but if this is in coding-conventions then people start using it.

By the way, thats remind one related item : in zee we set build_type to empty. With recent upstream Spack, we can't do that now and we have to fix it (either in bob or workaround in spack).

[ohm314]

We discussed this issue also today, along with #97 . Basically everyone agrees that the approach setting -Wall -Werror and selectively disabling certain errors/warnings makes sense and this will certainly remain in hpc-coding-conventions. @mgeplf brought up one point early in this threat, which I (and at least everyone who was in our HPC Team discussion) agree with: These "defensive" flags should be only used at development time. The default build flags (so when building Release or ReleaseWithDebInfo) should be robust, in the sense that we don't make the compiler stumble over every warning. For the average user, who just wants to build from source this would lead to unnecessary build issues. This is also because we cannot anticipate every compiler/version combo out there.
When building for development locally or in our CI, however, we want to have the all flags set to paranoid, to catch as many issues as possible.
As discussed in #97, this should be achieved without bob, to keep things simpler and make it easy for others to adopt these flags even for projects that don't use hpc-coding-conventsions (eg. NEURON, HighFive).