search

leggedrobotics / ros_best_practices

Best practices, conventions, and tricks for ROS
https://rsl.ethz.ch/education-students/lectures/ros.html
BSD 3-Clause "New" or "Revised" License
1.51k stars 415 forks source link

Added -Wall and -Werror to CmakeLists. #12

Closed remod closed 6 years ago

remod commented 6 years ago

This shows all build warnings and turns them into errors, meaning that you cannot build a package with warnings anymore. I introduced this for the ANYdrive SDK and could fix all warnings so far.

In my opinion this should be standard for all our packages.

Additional literature: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

pleemann commented 6 years ago

I aggree on adding it to the cmakelists. But I would not add it as default to the build server at this point, we will probably have a constant red wall otherwise.

pfankhauser commented 6 years ago

Thanks @remod! I agree with @pleemann, should not be default, at least not in this public repo. What do you think of adding it as commented-out line with a note on what this line does?

remod commented 6 years ago

I am fine with both, default and commented out.

However I would strongly recommend everyone at ANYbotics to use it. While adding it for the ANYdrive SDK, all code incl. dependencies was checked for warnings and I found unused variables, missing return statements and comparisons of signed and unsigned integers. In my opinion this is a small step towards more reliable code.

What do the others think?

pfankhauser commented 6 years ago

I fully agree, @remod. Let's have this mandatory for ANYbotics, but not in this public template (used also in the ROS course etc.), ok?