Optional Werror #459
Optional Werror #459
Conversation
| @@ -9,7 +9,6 @@ set(lcm_lua_sources | |||
|
|
|||
| add_library(lcm-lua MODULE ${lcm_lua_sources}) | |||
| set_target_properties(lcm-lua PROPERTIES OUTPUT_NAME "lcm" PREFIX "") | |||
| target_compile_options(lcm-lua PRIVATE -Werror -Wall -Wextra) | |||
There was a problem hiding this comment.
This line should only add -Werror -Wall -Wextra to the lcm-lua target. The PRIVATE should change only the target's COMPILE_OPTIONS (and not INTERFACE_COMPILE_OPTIONS). I turned this on because I fixed warnings in lcm-lua, and I wanted to make sure other people would not reintroduce warnings.
Can you please explain why you would want to use --compile-no-warning-as-error? I'm having a hard time thinking of a situation where this would be beneficial.
There was a problem hiding this comment.
I thought about this some more, and if we want to remove -Werror here we can do so, as long as -Werror is guaranteed to be turned on during CI. It would accomplish the same goal. Either way, I would prefer we keep the other options -Wall -Wextra here. (I do not really agree with the idea of local development using different flags from CI, but I understand the reasoning laid out in #457 (comment).)
There was a problem hiding this comment.
That makes a lot of sense. I agree that we should only remove the flags here that are in duplicate to what CI runs with, which excludes -Wall -Wextra. I've amended the original commit to 2b1d725.
|
I've caught up on #457. I must admit, I didn't realize that GCC needs optimization flags turned on for some warnings. Anyway, it seems after #458 that CI is now building a release build with It still seems to me that we should keep the warning flags for the |
nosracd
force-pushed
the
cmake/no_werror_by_default
branch
from
5572714
to
2b1d725
Compare
Unfortunately, it appears that if Werror is turned on in this fashion cmake won't let it be overridden via the usual command-line arguments:
-DCMAKE_C_FLAGS="-Wno-error" -DCMAKE_CXX_FLAGS="-Wno-error"--compile-no-warning-as-errorThis PR removes all arguments from
lcm-lua/CMakeLists.txtwhich modify the number of warnings and how they're treated, instead preferring opt-in command-line arguments to increase the number of warnings and treat them as errors.Resolves #457.