Add compiler option -Wconversion and add explicit casts for conversions that may alter the value or change the sign

[meyerj]

Follow-up on #261 (comment).

I think most implicit conversions have been okay. But not checking the signed return value of rcutils_vsnprintf() in format_string.c could be considered a bug.

[jacobperron]

LGTM

Testing a couple downstream packages along with rcutils:

• Linux
• Linux-aarch64
• macOS
• Windows

[jacobperron]

Looks like clang is a bit more picky.

We can fix the two warnings in the tests, but I'm not sure what to do about the warnings coming from the gtest files. Maybe we can ignore them some way?

[jacobperron]

@meyerj Can you fix this additional warning on macOS: https://ci.ros2.org/job/ci_osx/9381/clang/fileName.-1409726512/ ?

For the the warnings coming from gtest, I guess we should do something similar to rclcpp and add -Wno-sign-conversion for Clang:

https://github.com/ros2/rclcpp/blob/7e68d3549cb4162630446a1cadf62a4b2f64cc82/rclcpp/CMakeLists.txt#L28-L32

[meyerj]

@meyerj Can you fix this additional warning on macOS: https://ci.ros2.org/job/ci_osx/9381/clang/fileName.-1409726512/ ?

Done in 7db43f8. By adding -Wno-sign-conversion in CMakeLists.txt the other fixes would not have been required, but I verified that all warnings other than from gtest/gmock sources and headers have been fixed even with -Wsign-conversion enabled (on Linux with gcc 9.3).

[jacobperron]

By adding -Wno-sign-conversion in CMakeLists.txt the other fixes would not have been required

What if we just add -Wno-sign-conversion for Clang? E.g.

if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
  add_compile_options(-Wno-sign-conversion)
endif()

[jacobperron]

err, sorry. I think you meant that the additional fixes in 7db43f8 aren't necessary with the warning suppression?

[meyerj]

err, sorry. I think you meant that the additional fixes in 7db43f8 aren't necessary with the warning suppression?

They are not necessary to fix the build on macOS, but from #263 (comment) I understood that you asked me to fix them anyway, such that the only remaining sign conversion warnings would be produced by gtest and gmock.

I always considered -Wno-sign-conversion as a temporary workaround. Or do you think sign conversion are too pedantic and should remain ignored forever, such that the other changes are not required? In that case also the last sentence of the comments in rclcpp/CMakeLists.txt and now added to rcutils/CMakeLists.txt is a bit misleading.

[jacobperron]

They are not necessary to fix the build on macOS, but from #263 (comment) I understood that you asked me to fix them anyway, such that the only remaining sign conversion warnings would be produced by gtest and gmock.

Understood, this is what I asked 👍

I always considered -Wno-sign-conversion as a temporary workaround.

I agree, it is just a workaround to deal with the gtest/gmock errors.

Sorry for the confusion. Thanks for iterating!

macOS: