missing break in switch-case in config.cpp?

[fmauch]

Since in the CMakeLists.txt the compiler is set to pedandic and Werror, build fails on rviz_common/config.cpp:145 as there is no break in the case section when using gcc7 (gc 7.2.1 used here):

/home/felix/Downloads/ros2/src/ros2/rviz/rviz_common/src/rviz_common/config.cpp:145:9: error: this statement may fall through [-Werror=implicit-fallthrough=]
         }
         ^
/home/felix/Downloads/ros2/src/ros2/rviz/rviz_common/src/rviz_common/config.cpp:147:5: note: here
     case Value:
     ^~~~
At global scope:
cc1plus: error: unrecognized command line option ‘-Wno-gnu-zero-variadic-macro-arguments’ [-Werror]
cc1plus: all warnings being treated as errors```

I know that gcc7 is not officially supported, but this might still be unintentional? Otherwise a statement suppressing this warning or at least a comment why there is no break there would be nice I guess.

[mjbogusz]

I've encountered that too and I think this actually is a bug inherited from the mainline. GCC7 simply added implicit-fallthrough warning to it's -Wextra.

My understanding is that there should be a break; statement, as falling through to the last case will erroneously set the type of the Config object to Value (within .setValue()).

Another issue is the unrecognized command line option ‘-Wno-gnu-zero-variadic-macro-arguments’: this is solved by explicitly setting the project type in CMakeLists.txt:

project(rviz_common CXX)

(solution found here)

---

The two issues above are in fact unrelated to each other - I'm not sure whether it would be better to open a PR for both of them at once or separately?

[mjbogusz]

After fixing those issues I've hit another wall:

In file included from /usr/include/c++/7.2.1/ext/string_conversions.h:41:0,
                 from /usr/include/c++/7.2.1/bits/basic_string.h:6349,
                 from /usr/include/c++/7.2.1/string:52,
                 from /home/mjbogusz/documents/ros2/src/ros2/rviz/rviz2/src/main.cpp:31:
/usr/include/c++/7.2.1/cstdlib:75:15: fatal error: stdlib.h: No such file or directory.
 #include_next <stdlib.h>
               ^~~~~~~~~~
compilation terminated.

After setting CMAKE_VERBOSE_MAKEFILE to true and analyzing the compilation command I've noticed this little gem:

-isystem /usr/lib64/cmake/yaml-cpp/../../../include

That effectively makes /usr/include/stdlib.h go up on the list of possible stdlib.h files to include from cstdlib and therefore #include_next has no valid target (this helped me understand that).

I've traced it down to rviz_yaml_cpp_vendor triggering environment hooks and setting additional include directories even if yaml-cpp is found on the system. I've fixed that by wrapping the whole download/build/hook/package block in an if statement:

find_package(yaml-cpp QUIET)
if(NOT yaml-cpp_FOUND)
    (...)
else()
    ament_package()
endif()

To ensure proper linking of yaml-cpp however the libraries for rviz_yaml_cpp_vendor had to be set so I've added another cmake 'extras' file doing just that.

I'll create a PR soon.

[racko]

Looking forward to your PR :)

btw: I noticed that /usr/lib64/cmake/yaml-cpp/../../../include is actually provided by the system's /usr/lib/cmake/yaml-cpp/yaml-cpp-config.cmake. Line 8 says set(YAML_CPP_INCLUDE_DIR "${YAML_CPP_CMAKE_DIR}/../../../include").

[Theosakamg]

The break is needed ?

From https://developers.redhat.com/blog/2017/03/10/wimplicit-fallthrough-in-gcc-7/
I use regex /* FALLTHRU */

[racko]

I created the following pull requests to fix the issues listed here: #157, #158, #159.

[mjbogusz]

@Theosakamg yes, the break is indeed needed there. It's an upstream bug.

@racko I have a slightly more complete solution for the yaml-cpp thingy but I want to clean it up and test it first.

[wjwwood]

Since the originally reported issue has been resolved in #158 I'm going to close this.