refactor: Add clang-tidy checks (#195)

bjsowa · web-flow · commit eef3dc264065 · 2026-01-12T03:45:49.000+01:00
* chore: Add ament_clang_tidy check for rolling

* chore: Add CMAKE_ARGS to enable compile commands in CI workflow

* Configure clang-tidy to check naming conventions

* Fix naming convention errors

* Add misc-include-cleaner check

* Enable google checks

* Enable other readability checks

* Remove cpplint

* Enable other misc checks

* Reformat

* Set clang-tidy jobs to processor count

* Fix formatting

* Suppress clang-analyzer warning caused by pluginlib

* Fix uncrustify for humble

* Add ENABLE_CLANG_TIDY option to CMake and update CI

* Update CI workflow

* Try to use single step for industrial ci
diff --git a/.clang-tidy b/.clang-tidy
@@ -0,0 +1,38 @@
+---
+Checks: >
+  clang-analyzer-*,
+  -clang-analyzer-optin.cplusplus.VirtualCall,
+  clang-diagnostic-*,
+  google-*,
+  misc-*,
+  -misc-non-private-member-variables-in-classes,
+  readability-*,
+  -readability-identifier-length,
+  -readability-magic-numbers,
+
+CheckOptions:
+  misc-include-cleaner.IgnoreHeaders: bits/chrono.h
+  readability-function-cognitive-complexity.IgnoreMacros: true
+  readability-identifier-naming.ClassCase: CamelCase
+  readability-identifier-naming.ClassMemberCase: lower_case
+  readability-identifier-naming.ConstexprVariableCase: CamelCase
+  readability-identifier-naming.EnumCase: CamelCase
+  readability-identifier-naming.EnumConstantCase: CamelCase
+  readability-identifier-naming.FunctionCase: lower_case
+  readability-identifier-naming.GlobalConstantCase: CamelCase
+  readability-identifier-naming.StaticConstantCase: lower_case
+  readability-identifier-naming.StaticVariableCase: lower_case
+  readability-identifier-naming.MacroDefinitionCase: UPPER_CASE
+  readability-identifier-naming.MacroDefinitionIgnoredRegexp: '^[A-Z]+(_[A-Z]+)*_$'
+  readability-identifier-naming.PrivateMemberCase: lower_case
+  readability-identifier-naming.ProtectedMemberCase: lower_case
+  readability-identifier-naming.PublicMemberCase: lower_case
+  readability-identifier-naming.PrivateMemberSuffix: _
+  readability-identifier-naming.ProtectedMemberSuffix: _
+  readability-identifier-naming.PublicMemberSuffix: ''
+  readability-identifier-naming.NamespaceCase: lower_case
+  readability-identifier-naming.ParameterCase: lower_case
+  readability-identifier-naming.TypeAliasCase: CamelCase
+  readability-identifier-naming.TypedefCase: CamelCase
+  readability-identifier-naming.VariableCase: lower_case
+  readability-identifier-naming.IgnoreMainLikeFunctions: true
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -20,6 +20,7 @@ jobs:
     env:
       ROS_DISTRO: ${{ matrix.ROS_DISTRO }}
       ROS_REPO: ${{ matrix.ROS_REPO }}
+      CMAKE_ARGS: ${{ matrix.ROS_DISTRO == 'rolling' && '-DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DENABLE_CLANG_TIDY=ON' || '' }}
     steps:
       - name: Checkout repo
         uses: actions/checkout@v4
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -165,6 +165,17 @@ if(BUILD_TESTING)
     )
   endif()
 
+  option(ENABLE_CLANG_TIDY "Enable ament_clang_tidy test" OFF)
+  if(ENABLE_CLANG_TIDY)
+    include(ProcessorCount)
+    processorcount(N)
+    set(ament_cmake_clang_tidy_JOBS ${N})
+  else()
+    list(APPEND AMENT_LINT_AUTO_EXCLUDE
+      ament_cmake_clang_tidy
+    )
+  endif()
+
   ament_lint_auto_find_test_dependencies()
 endif()
 
diff --git a/include/web_video_server/multipart_stream.hpp b/include/web_video_server/multipart_stream.hpp
@@ -56,7 +56,7 @@ struct PendingFooter
 class MultipartStream
 {
 public:
-  MultipartStream(
+  explicit MultipartStream(
     async_web_server_cpp::HttpConnectionPtr & connection,
     const std::string & boundary = "boundarydonotcross",
     std::size_t max_queue_size = 1);
@@ -77,7 +77,6 @@ class MultipartStream
 private:
   bool is_busy();
 
-private:
   const std::size_t max_queue_size_;
   async_web_server_cpp::HttpConnectionPtr connection_;
   std::string boundary_;
diff --git a/include/web_video_server/streamers/image_transport_streamer.hpp b/include/web_video_server/streamers/image_transport_streamer.hpp
@@ -70,8 +70,10 @@ class ImageTransportStreamerBase : public StreamerBase
 
 protected:
   virtual cv::Mat decode_image(const sensor_msgs::msg::Image::ConstSharedPtr & msg);
-  virtual void send_image(const cv::Mat &, const std::chrono::steady_clock::time_point & time) = 0;
-  virtual void initialize(const cv::Mat &);
+  virtual void send_image(
+    const cv::Mat & img,
+    const std::chrono::steady_clock::time_point & time) = 0;
+  virtual void initialize(const cv::Mat & img);
 
   image_transport::Subscriber image_sub_;
   int output_width_;
@@ -81,7 +83,7 @@ class ImageTransportStreamerBase : public StreamerBase
   std::string qos_profile_name_;
 
   std::chrono::steady_clock::time_point last_frame_;
-  cv::Mat output_size_image;
+  cv::Mat output_size_image_;
   std::mutex send_mutex_;
 
 private:
diff --git a/include/web_video_server/streamers/jpeg_streamers.hpp b/include/web_video_server/streamers/jpeg_streamers.hpp
@@ -59,7 +59,7 @@ class MjpegStreamer : public ImageTransportStreamerBase
   ~MjpegStreamer();
 
 protected:
-  virtual void send_image(const cv::Mat &, const std::chrono::steady_clock::time_point & time);
+  virtual void send_image(const cv::Mat & img, const std::chrono::steady_clock::time_point & time);
 
 private:
   MultipartStream stream_;
@@ -86,7 +86,7 @@ class JpegSnapshotStreamer : public ImageTransportStreamerBase
   ~JpegSnapshotStreamer();
 
 protected:
-  virtual void send_image(const cv::Mat &, const std::chrono::steady_clock::time_point & time);
+  virtual void send_image(const cv::Mat & img, const std::chrono::steady_clock::time_point & time);
 
 private:
   int quality_;
diff --git a/include/web_video_server/streamers/libav_streamer.hpp b/include/web_video_server/streamers/libav_streamer.hpp
@@ -78,8 +78,8 @@ class LibavStreamerBase : public ImageTransportStreamerBase
 
 protected:
   virtual void initialize_encoder() = 0;
-  virtual void send_image(const cv::Mat &, const std::chrono::steady_clock::time_point & time);
-  virtual void initialize(const cv::Mat &);
+  virtual void send_image(const cv::Mat & img, const std::chrono::steady_clock::time_point & time);
+  virtual void initialize(const cv::Mat & img);
   AVFormatContext * format_context_;
   const AVCodec * codec_;
   AVCodecContext * codec_context_;
diff --git a/include/web_video_server/streamers/png_streamers.hpp b/include/web_video_server/streamers/png_streamers.hpp
@@ -59,7 +59,7 @@ class PngStreamer : public ImageTransportStreamerBase
   ~PngStreamer();
 
 protected:
-  virtual void send_image(const cv::Mat &, const std::chrono::steady_clock::time_point & time);
+  virtual void send_image(const cv::Mat & img, const std::chrono::steady_clock::time_point & time);
   virtual cv::Mat decode_image(const sensor_msgs::msg::Image::ConstSharedPtr & msg);
 
 private:
@@ -87,7 +87,7 @@ class PngSnapshotStreamer : public ImageTransportStreamerBase
   ~PngSnapshotStreamer();
 
 protected:
-  virtual void send_image(const cv::Mat &, const std::chrono::steady_clock::time_point & time);
+  virtual void send_image(const cv::Mat & img, const std::chrono::steady_clock::time_point & time);
   virtual cv::Mat decode_image(const sensor_msgs::msg::Image::ConstSharedPtr & msg);
 
 private:
diff --git a/include/web_video_server/streamers/ros_compressed_streamer.hpp b/include/web_video_server/streamers/ros_compressed_streamer.hpp
@@ -63,15 +63,15 @@ class RosCompressedStreamer : public StreamerBase
 
 protected:
   virtual void send_image(
-    const sensor_msgs::msg::CompressedImage::ConstSharedPtr msg,
+    sensor_msgs::msg::CompressedImage::ConstSharedPtr msg,
     const std::chrono::steady_clock::time_point & time);
 
 private:
-  void image_callback(const sensor_msgs::msg::CompressedImage::ConstSharedPtr msg);
+  void image_callback(sensor_msgs::msg::CompressedImage::ConstSharedPtr msg);
   MultipartStream stream_;
   rclcpp::Subscription<sensor_msgs::msg::CompressedImage>::SharedPtr image_sub_;
   std::chrono::steady_clock::time_point last_frame_;
-  sensor_msgs::msg::CompressedImage::ConstSharedPtr last_msg;
+  sensor_msgs::msg::CompressedImage::ConstSharedPtr last_msg_;
   std::mutex send_mutex_;
   std::string qos_profile_name_;
 };
@@ -100,11 +100,11 @@ class RosCompressedSnapshotStreamer : public StreamerBase
 
 protected:
   virtual void send_image(
-    const sensor_msgs::msg::CompressedImage::ConstSharedPtr msg,
+    sensor_msgs::msg::CompressedImage::ConstSharedPtr msg,
     const std::chrono::steady_clock::time_point & time);
 
 private:
-  void image_callback(const sensor_msgs::msg::CompressedImage::ConstSharedPtr msg);
+  void image_callback(sensor_msgs::msg::CompressedImage::ConstSharedPtr msg);
 
   rclcpp::Subscription<sensor_msgs::msg::CompressedImage>::SharedPtr image_sub_;
   std::string qos_profile_name_;
diff --git a/include/web_video_server/utils.hpp b/include/web_video_server/utils.hpp
@@ -42,6 +42,6 @@ namespace web_video_server
  * @param name The name of the QoS profile name.
  * @return An optional containing the matching QoS profile.
  */
-std::optional<rmw_qos_profile_t> get_qos_profile_from_name(const std::string name);
+std::optional<rmw_qos_profile_t> get_qos_profile_from_name(std::string name);
 
 }  // namespace web_video_server
diff --git a/include/web_video_server/web_video_server.hpp b/include/web_video_server/web_video_server.hpp
@@ -102,7 +102,6 @@ class WebVideoServer : public rclcpp::Node
   rclcpp::TimerBase::SharedPtr cleanup_timer_;
 
   // Parameters
-  int ros_threads_;
   double publish_rate_;
   int port_;
   std::string address_;
diff --git a/package.xml b/package.xml
@@ -36,8 +36,8 @@
   <exec_depend>rclcpp_components</exec_depend>
 
   <test_depend>ament_lint_auto</test_depend>
+  <test_depend>ament_cmake_clang_tidy</test_depend>
   <test_depend>ament_cmake_copyright</test_depend>
-  <test_depend>ament_cmake_cpplint</test_depend>
   <test_depend>ament_cmake_lint_cmake</test_depend>
   <test_depend>ament_cmake_xmllint</test_depend>
   <test_depend>ament_cmake_uncrustify</test_depend>
diff --git a/src/multipart_stream.cpp b/src/multipart_stream.cpp
@@ -76,7 +76,7 @@ void MultipartStream::send_part_header(
   snprintf(
     stamp, sizeof(stamp), "%.06lf",
     std::chrono::duration_cast<std::chrono::duration<double>>(time.time_since_epoch()).count());
-  std::shared_ptr<std::vector<async_web_server_cpp::HttpHeader>> headers(
+  const std::shared_ptr<std::vector<async_web_server_cpp::HttpHeader>> headers(
     new std::vector<async_web_server_cpp::HttpHeader>());
   headers->push_back(async_web_server_cpp::HttpHeader("Content-type", type));
   headers->push_back(async_web_server_cpp::HttpHeader("X-Timestamp", stamp));
@@ -88,7 +88,7 @@ void MultipartStream::send_part_header(
 
 void MultipartStream::send_part_footer(const std::chrono::steady_clock::time_point & time)
 {
-  std::shared_ptr<std::string> str(new std::string("\r\n--" + boundary_ + "\r\n"));
+  const std::shared_ptr<std::string> str(new std::string("\r\n--" + boundary_ + "\r\n"));
   PendingFooter pf;
   pf.timestamp = time;
   pf.contents = str;
@@ -136,7 +136,7 @@ bool MultipartStream::is_busy()
       }
     }
   }
-  return !(max_queue_size_ == 0 || pending_footers_.size() < max_queue_size_);
+  return max_queue_size_ != 0 && pending_footers_.size() >= max_queue_size_;
 }
 
 }  // namespace web_video_server
diff --git a/src/streamer.cpp b/src/streamer.cpp
@@ -32,6 +32,8 @@
 
 #include <vector>
 #include <sstream>
+#include <string>
+#include <utility>
 
 #include "rclcpp/node.hpp"
 #include "rclcpp/logging.hpp"
diff --git a/src/streamers/h264_streamer.cpp b/src/streamers/h264_streamer.cpp
@@ -78,7 +78,7 @@ void H264Streamer::initialize_encoder()
   av_opt_set_int(codec_context_->priv_data, "g", 1, 0);
 
   // container format options
-  if (!strcmp(format_context_->oformat->name, "mp4")) {
+  if (strcmp(format_context_->oformat->name, "mp4") == 0) {
     // set up mp4 for streaming (instead of seekable file output)
     av_dict_set(&opt_, "movflags", "+frag_keyframe+empty_moov+faststart", 0);
   }
diff --git a/src/streamers/image_transport_streamer.cpp b/src/streamers/image_transport_streamer.cpp
@@ -104,6 +104,12 @@ ImageTransportStreamerBase::~ImageTransportStreamerBase()
 {
 }
 
+// We disable deprecation warnings for image_transport API usage
+// to maintain compatibility with older ROS 2 distributions.
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+// NOLINTBEGIN(clang-diagnostic-deprecated-declarations)
+
 void ImageTransportStreamerBase::start()
 {
   auto node = lock_node();
@@ -112,15 +118,15 @@ void ImageTransportStreamerBase::start()
     return;
   }
 
-  image_transport::TransportHints hints(node.get(), default_transport_);
+  const image_transport::TransportHints hints(node.get(), default_transport_);
   auto tnat = node->get_topic_names_and_types();
   inactive_ = true;
   for (auto topic_and_types : tnat) {
     if (topic_and_types.second.size() > 1) {
       // skip over topics with more than one type
       continue;
     }
-    auto & topic_name = topic_and_types.first;
+    const auto & topic_name = topic_and_types.first;
     if (topic_name == topic_ || (topic_name.find("/") == 0 && topic_name.substr(1) == topic_)) {
       inactive_ = false;
       break;
@@ -147,7 +153,10 @@ void ImageTransportStreamerBase::start()
     default_transport_, qos_profile.value());
 }
 
-void ImageTransportStreamerBase::initialize(const cv::Mat &)
+#pragma GCC diagnostic pop
+// NOLINTEND(clang-diagnostic-deprecated-declarations)
+
+void ImageTransportStreamerBase::initialize(const cv::Mat & /*img*/)
 {
 }
 
@@ -163,7 +172,7 @@ void ImageTransportStreamerBase::restream_frame(std::chrono::duration<double>/*
     return;
   }
 
-  try_send_image(output_size_image, last_frame_, *node);
+  try_send_image(output_size_image_, last_frame_, *node);
 }
 
 void ImageTransportStreamerBase::image_callback(const sensor_msgs::msg::Image::ConstSharedPtr & msg)
@@ -181,8 +190,8 @@ void ImageTransportStreamerBase::image_callback(const sensor_msgs::msg::Image::C
   cv::Mat img;
   try {
     img = decode_image(msg);
-    int input_width = img.cols;
-    int input_height = img.rows;
+    const int input_width = img.cols;
+    const int input_height = img.rows;
 
     if (output_width_ == -1) {
       output_width_ = input_width;
@@ -193,22 +202,22 @@ void ImageTransportStreamerBase::image_callback(const sensor_msgs::msg::Image::C
 
     if (invert_) {
       // Rotate 180 degrees
-      cv::flip(img, img, false);
-      cv::flip(img, img, true);
+      cv::flip(img, img, 0);
+      cv::flip(img, img, 1);
     }
 
-    std::scoped_lock lock(send_mutex_);  // protects output_size_image
+    const std::scoped_lock lock(send_mutex_);  // protects output_size_image_
     if (output_width_ != input_width || output_height_ != input_height) {
       cv::Mat img_resized;
-      cv::Size new_size(output_width_, output_height_);
+      const cv::Size new_size(output_width_, output_height_);
       cv::resize(img, img_resized, new_size);
-      output_size_image = img_resized;
+      output_size_image_ = img_resized;
     } else {
-      output_size_image = img;
+      output_size_image_ = img;
     }
 
     if (!initialized_) {
-      initialize(output_size_image);
+      initialize(output_size_image_);
       initialized_ = true;
     }
 
@@ -225,7 +234,7 @@ void ImageTransportStreamerBase::image_callback(const sensor_msgs::msg::Image::C
     return;
   }
 
-  try_send_image(output_size_image, last_frame_, *node);
+  try_send_image(output_size_image_, last_frame_, *node);
 }
 
 void ImageTransportStreamerBase::try_send_image(
@@ -234,7 +243,7 @@ void ImageTransportStreamerBase::try_send_image(
   rclcpp::Node & node)
 {
   try {
-    std::scoped_lock lock(send_mutex_);
+    const std::scoped_lock lock(send_mutex_);
     send_image(img, std::chrono::steady_clock::now());
   } catch (boost::system::system_error & e) {
     // happens when client disconnects
@@ -259,7 +268,7 @@ cv::Mat ImageTransportStreamerBase::decode_image(
 {
   if (msg->encoding.find("F") != std::string::npos) {
     // scale floating point images
-    cv::Mat float_image_bridge = cv_bridge::toCvCopy(msg, msg->encoding)->image;
+    const cv::Mat float_image_bridge = cv_bridge::toCvCopy(msg, msg->encoding)->image;
     cv::Mat_<float> float_image = float_image_bridge;
     double max_val;
     cv::minMaxIdx(float_image, 0, &max_val);
@@ -268,10 +277,9 @@ cv::Mat ImageTransportStreamerBase::decode_image(
       float_image *= (255 / max_val);
     }
     return float_image;
-  } else {
-    // Convert to OpenCV native BGR color
-    return cv_bridge::toCvCopy(msg, "bgr8")->image;
   }
+  // Convert to OpenCV native BGR color
+  return cv_bridge::toCvCopy(msg, "bgr8")->image;
 }
 
 std::vector<std::string> ImageTransportStreamerFactoryBase::get_available_topics(
diff --git a/src/streamers/jpeg_streamers.cpp b/src/streamers/jpeg_streamers.cpp
diff --git a/src/streamers/libav_streamer.cpp b/src/streamers/libav_streamer.cpp
diff --git a/src/streamers/png_streamers.cpp b/src/streamers/png_streamers.cpp
diff --git a/src/streamers/ros_compressed_streamer.cpp b/src/streamers/ros_compressed_streamer.cpp
diff --git a/src/streamers/vp8_streamer.cpp b/src/streamers/vp8_streamer.cpp
diff --git a/src/utils.cpp b/src/utils.cpp
diff --git a/src/web_video_server.cpp b/src/web_video_server.cpp
diff --git a/src/web_video_server_node.cpp b/src/web_video_server_node.cpp
