Serious Compiler Warning from yabloc_image_processing

[andoh104]

Checklist

• I've read the contribution guidelines.
• I've searched other issues and no duplicate issues were found.
• I'm convinced that this is not my fault but a bug.

Description

This code is causing a serious compiler warning. I've confirmed that the code itself works fine, but disabling this warning is problematic.

Expected behavior

No serious warnings will be generated.

Actual behavior

--- stderr: yabloc_image_processing            
CMake Warning (dev) at CMakeLists.txt:15 (find_package):
  Policy CMP0074 is not set: find_package uses <PackageName>_ROOT variables.
  Run "cmake --help-policy CMP0074" for policy details.  Use the cmake_policy
  command to set the policy and suppress this warning.
  CMake variable PCL_ROOT is set to:
    /usr
  For compatibility, CMake is ignoring the variable.
This warning is for project developers.  Use -Wno-dev to suppress it.
In this package, headers install destination is set to `include` by ament_auto_package. It is recommended to install `include/yabloc_image_processing` instead and will be the default behavior of ament_auto_package from ROS 2 Kilted Kaiju. On distributions before Kilted, ament_auto_package behaves the same way when you use USE_SCOPED_HEADER_INSTALL_DIR option.
/hoge/src/autoware/universe/localization/yabloc/yabloc_image_processing/src/lanelet2_overlay/lanelet2_overlay_core.cpp: In member function ‘void yabloc::lanelet2_overlay::Lanelet2Overlay::_ZN6yabloc16lanelet2_overlay15Lanelet2Overlay26draw_overlay_line_segmentsERN2cv3MatERKN13geometry_msgs3msg5Pose_ISaIvEEERKN3pcl10PointCloudINSC_11PointNormalEEE.part.0(cv::Mat&, const Pose&, const LineSegments&)’:
/hoge/src/autoware/universe/localization/yabloc/yabloc_image_processing/src/lanelet2_overlay/lanelet2_overlay_core.cpp:161:21: error: ‘*(float*)((char*)&uv2 + offsetof(Eigen::Vector3f, Eigen::Matrix<float, 3, 1, 0, 3, 1>::<unnamed>.Eigen::PlainObjectBase<Eigen::Matrix<float, 3, 1, 0, 3, 1> >::<unnamed>.Eigen::MatrixBase<Eigen::Matrix<float, 3, 1, 0, 3, 1> >::<unnamed>.Eigen::DenseBase<Eigen::Matrix<float, 3, 1, 0, 3, 1> >::<unnamed>.Eigen::DenseCoeffsBase<Eigen::Matrix<float, 3, 1, 0, 3, 1>, 3>::<unnamed>.Eigen::DenseCoeffsBase<Eigen::Matrix<float, 3, 1, 0, 3, 1>, 1>::<unnamed>.Eigen::DenseCoeffsBase<Eigen::Matrix<float, 3, 1, 0, 3, 1>, 0>::<unnamed>))’ is used uninitialized [-Werror=uninitialized]
  161 |     Eigen::Vector3f uv2;
      |                     ^~~
/hoge/src/autoware/universe/localization/yabloc/yabloc_image_processing/src/lanelet2_overlay/lanelet2_overlay_core.cpp:161:21: error: ‘uv2.Eigen::Matrix<float, 3, 1, 0, 3, 1>::<unnamed>.Eigen::PlainObjectBase<Eigen::Matrix<float, 3, 1, 0, 3, 1> >::m_storage.Eigen::DenseStorage<float, 3, 3, 1, 0>::m_data.Eigen::internal::plain_array<float, 3, 0, 0>::array[1]’ is used uninitialized [-Werror=uninitialized]
/hoge/src/autoware/universe/localization/yabloc/yabloc_image_processing/src/lanelet2_overlay/lanelet2_overlay_core.cpp:160:21: error: ‘*(float*)((char*)&uv1 + offsetof(Eigen::Vector3f, Eigen::Matrix<float, 3, 1, 0, 3, 1>::<unnamed>.Eigen::PlainObjectBase<Eigen::Matrix<float, 3, 1, 0, 3, 1> >::<unnamed>.Eigen::MatrixBase<Eigen::Matrix<float, 3, 1, 0, 3, 1> >::<unnamed>.Eigen::DenseBase<Eigen::Matrix<float, 3, 1, 0, 3, 1> >::<unnamed>.Eigen::DenseCoeffsBase<Eigen::Matrix<float, 3, 1, 0, 3, 1>, 3>::<unnamed>.Eigen::DenseCoeffsBase<Eigen::Matrix<float, 3, 1, 0, 3, 1>, 1>::<unnamed>.Eigen::DenseCoeffsBase<Eigen::Matrix<float, 3, 1, 0, 3, 1>, 0>::<unnamed>))’ is used uninitialized [-Werror=uninitialized]
  160 |     Eigen::Vector3f uv1;
      |                     ^~~
/hoge/src/autoware/universe/localization/yabloc/yabloc_image_processing/src/lanelet2_overlay/lanelet2_overlay_core.cpp:160:21: error: ‘uv1.Eigen::Matrix<float, 3, 1, 0, 3, 1>::<unnamed>.Eigen::PlainObjectBase<Eigen::Matrix<float, 3, 1, 0, 3, 1> >::m_storage.Eigen::DenseStorage<float, 3, 3, 1, 0>::m_data.Eigen::internal::plain_array<float, 3, 0, 0>::array[1]’ is used uninitialized [-Werror=uninitialized]
cc1plus: all warnings being treated as errors
gmake[2]: *** [CMakeFiles/yabloc_image_processing.dir/build.make:160: CMakeFiles/yabloc_image_processing.dir/src/lanelet2_overlay/lanelet2_overlay_core.cpp.o] エラー 1
gmake[1]: *** [CMakeFiles/Makefile2:149: CMakeFiles/yabloc_image_processing.dir/all] エラー 2
gmake: *** [Makefile:146: all] エラー 2
---

Steps to reproduce

colcon build --symlink-install --cmake-args "-DCMAKE_CXX_FLAGS=-Wno-error=maybe-uninitialized -Wno-error=array-bounds -Wno-error=nonnull" "-DCMAKE_BUILD_TYPE=Release"

Versions

No response

Possible causes

Below are Copilot's suggested fixes. I think the former is sufficient.

Let's analyze the highlighted snippet:

Eigen::Vector3f uv1;
Eigen::Vector3f uv2;
if (p1_is_visible) uv1 = from_camera1 / from_camera1.z();
if (p2_is_visible) uv2 = from_camera2 / from_camera2.z();

1. Code Smells or Antipatterns

• Uninitialized Variable Use: If p1_is_visible or p2_is_visible is false, then uv1 or uv2 will remain uninitialized, which may lead to undefined behavior if they are used later without being explicitly set. This is a common code smell known as "uninitialized variable."
• Potential for Implicit Initialization: In C++, declaring Eigen::Vector3f uv1; without initialization does not guarantee zero-initialization. If the variable is later used without being set, bugs can be hard to track.
• Readability: The if-statements are terse and rely on the reader to notice that either variable might not be set, which can hinder maintainability.

2. Recommendations and Example Improvements

• Explicit Initialization: Initialize uv1 and uv2 to a safe default value (e.g., zero vector) to prevent undefined behavior.
• Make Assignment More Clear: Consider using an else clause or initializing within the declaration to clarify intent.
• Comment the Logic: Brief comments can help future maintainers understand why these are conditionally set.

Improved Example:

Eigen::Vector3f uv1 = Eigen::Vector3f::Zero();
Eigen::Vector3f uv2 = Eigen::Vector3f::Zero();
if (p1_is_visible) {
    uv1 = from_camera1 / from_camera1.z();
}
if (p2_is_visible) {
    uv2 = from_camera2 / from_camera2.z();
}
// Now uv1 and uv2 are always initialized, minimizing risk of undefined values.

Or, if the use of uv1 and uv2 later depends strictly on the visibility, consider restructuring:

std::optional<Eigen::Vector3f> uv1;
std::optional<Eigen::Vector3f> uv2;
if (p1_is_visible) {
    uv1 = from_camera1 / from_camera1.z();
}
if (p2_is_visible) {
    uv2 = from_camera2 / from_camera2.z();
}
// Later, check uv1.has_value() before using.

3. C++ Best Practices

• Avoid Uninitialized Local Variables: Especially with types like Eigen vectors, always initialize variables unless performance absolutely demands otherwise and correctness is assured.
• Prefer Explicitness: C++ code is often easier to maintain when initialization and assignment are straightforward and obvious.

---

Summary:
The main issue here is the risk of using uninitialized variables. By initializing uv1 and uv2 to zero or using std::optional, you make the code safer and more maintainable, following modern C++ best practices.

Additional context

No response