Add clang-tidy to CI with an initial set of checks

.cirrus.yml

@@ -54,6 +54,14 @@ unix_env: &UNIX_ENV
 # Linux EOL timelines: https://linuxlifecycle.com/
 # Fedora (~13 months): https://fedoraproject.org/wiki/Fedora_Release_Life_Cycle
 
+clang_tidy_task:
+  container:
+    dockerfile: ci/fedora-36/Dockerfile
+    << : *RESOURCES_TEMPLATE
+  sync_submodules_script: git submodule update --recursive --init
+  build_script: ./ci/analyze.sh
+  << : *UNIX_ENV
+
 fedora35_task:
   container:
     # Fedora 35 EOL: Around Dec 2022

.clang-tidy

@@ -0,0 +1,10 @@
+---
+  Checks: -*,
+          bugprone-*,
+          clang-analyzer-*,
+          -bugprone-easily-swappable-parameters,
+          -clang-analyzer-security.insecureAPI.rand,
+          -clang-analyzer-webkit*,
+  WarningsAsErrors: '*'
+  HeaderFilterRegex: 'broker/*.hh'
+...

CMakeLists.txt

@@ -398,6 +398,23 @@ else()
   set(BROKER_LIBRARY broker_static)
 endif()
 
+set(tidyCfgFile "${CMAKE_SOURCE_DIR}/.clang-tidy")
+if (BROKER_ENABLE_TIDY)
+  # Create a preprocessor definition that depends on .clang-tidy content so
+  # the compile command will change when .clang-tidy changes. This ensures
+  # that a subsequent build re-runs clang-tidy on all sources even if they
+  # do not otherwise need to be recompiled.
+  file(SHA1 ${CMAKE_CURRENT_SOURCE_DIR}/.clang-tidy clang_tidy_sha1)
+  set(BROKER_CLANG_TIDY_DEF "CLANG_TIDY_SHA1=${clang_tidy_sha1}")
+  unset(clang_tidy_sha1)
+  add_custom_target(clang_tidy_dummy DEPENDS ${tidyCfgFile})
+  set_target_properties(${BROKER_LIBRARY} PROPERTIES
+                        CXX_CLANG_TIDY "clang-tidy;--config-file=${tidyCfgFile}")
+  target_compile_definitions(${BROKER_LIBRARY} PRIVATE ${BROKER_CLANG_TIDY_DEF})
+  configure_file(.clang-tidy .clang-tidy COPYONLY)
+endif ()
+
+
 # -- Tools --------------------------------------------------------------------
 
 macro(add_tool name)
@@ -409,6 +426,11 @@ macro(add_tool name)
     target_link_libraries(${name} broker_static CAF::core CAF::io CAF::net)
     add_dependencies(${name} broker_static)
   endif()
+  if (BROKER_ENABLE_TIDY)
+    set_target_properties(${name} PROPERTIES
+                          CXX_CLANG_TIDY "clang-tidy;--config-file=${tidyCfgFile}")
+    target_compile_definitions(${name} PRIVATE ${BROKER_CLANG_TIDY_DEF})
+  endif ()
 endmacro()
 
 if (NOT BROKER_DISABLE_TOOLS)

ci/analyze.sh

@@ -0,0 +1,8 @@
+#! /usr/bin/env bash
+
+set -e
+set -x
+
+CXX=clang++ ./configure --build-type=debug
+
+run-clang-tidy -p build -j ${BROKER_CI_CPUS} $PWD/src

ci/fedora-36/Dockerfile

@@ -5,6 +5,8 @@ FROM fedora:36
 ENV DOCKERFILE_VERSION 20220615
 
 RUN dnf -y install \
+    clang \
+    clang-tools-extra \
     cmake \
     diffutils \
     gcc \

configure

@@ -24,6 +24,7 @@ Usage: $0 [OPTION]... [VAR=VALUE]...
     --enable-static-only   only build static libraries, not shared
     --with-log-level=LVL   build embedded CAF with debugging output.  Levels:
                              ERROR, WARNING, INFO, DEBUG, TRACE
+    --with-clang-tidy      run with clang-tidy (requires CMake >= 3.7.2)
     --sanitizers=LIST      comma-separated list of sanitizer names to enable
     --disable-sanitizer-optimizations
                            build without any optimizations when using sanitizers
@@ -131,6 +132,9 @@ while [ $# -ne 0 ]; do
         --enable-static)
             append_cache_entry ENABLE_STATIC        BOOL   true
             ;;
+        --with-clang-tidy)
+            append_cache_entry BROKER_ENABLE_TIDY   BOOL   true
+            ;;
         --sanitizers=*)
             append_cache_entry BROKER_SANITIZERS    STRING    $optarg
             ;;

include/broker/detail/prefix_matcher.hh

@@ -11,10 +11,10 @@ namespace broker::detail {
 struct prefix_matcher {
   using filter_type = std::vector<topic>;
 
-  bool operator()(const filter_type& filter, const topic& t) const;
+  bool operator()(const filter_type& filter, const topic& t) const noexcept;
 
   template <class T>
-  bool operator()(const filter_type& filter, const T& x) const {
+  bool operator()(const filter_type& filter, const T& x) const noexcept {
     return (*this)(filter, get_topic(x));
   }
 };

include/broker/status.hh

@@ -39,7 +39,7 @@ template <sc S>
 using sc_constant = std::integral_constant<sc, S>;
 
 /// @relates sc
-std::string to_string(sc code) noexcept;
+std::string to_string(sc code);
 
 /// @relates sc
 bool convert(std::string_view str, sc& code) noexcept;
@@ -229,10 +229,10 @@ public:
 
   /// @copydoc status::code
   /// @pre `valid()`
-  sc code() const noexcept;
+  sc code() const;
 
   /// @copydoc status::code
-  const std::string* message() const noexcept;
+  const std::string* message() const;
 
   /// Retrieves additional contextual information, if available.
   std::optional<endpoint_info> context() const;

include/broker/worker.hh

@@ -4,6 +4,7 @@
 #include "broker/detail/comparable.hh"
 
 #include <cstddef>
+#include <cstdint>
 #include <functional>
 #include <string>
 #include <utility>
@@ -63,7 +64,7 @@ public:
 
   /// Compares this instance to `other`.
   /// @returns -1 if `*this < other`, 0 if `*this == other`, and 1 otherwise.
-  int compare(const worker& other) const noexcept;
+  intptr_t compare(const worker& other) const noexcept;
 
   /// Returns a has value for the ID.
   size_t hash() const noexcept;

src/address.cc

@@ -97,7 +97,7 @@ bool address::mask(uint8_t top_bits_to_keep) {
   if (top_bits_to_keep > 128)
     return false;
   uint32_t mask[4] = {0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff};
-  auto res = std::ldiv(top_bits_to_keep, 32);
+  auto res = std::div(top_bits_to_keep, 32);
   if (res.quot < 4)
     mask[res.quot] =
       caf::detail::to_network_order(mask[res.quot] & ~bit_mask32(32 - res.rem));

src/broker-node.cc

@@ -82,12 +82,6 @@ bool convert(const caf::uri& from, network_info& to) {
   const auto& auth = from.authority();
   if (auth.empty())
     return false;
-  auto set_host = [&](const auto& host) {
-    if constexpr (std::is_same_v<decltype(host), const std::string&>)
-      to.address = host;
-    else
-      to.address = to_string(host);
-  };
   to.port = auth.port;
   return true;
 }
@@ -308,9 +302,9 @@ void relay_mode(broker::endpoint& ep, topic_list topics) {
     timeout += std::chrono::seconds(1);
     size_t received = 0;
     for (;;) {
-      auto x = in.get(timeout);
-      if (x) {
-        if (!handle_message(*x))
+      if (auto maybe_msg = in.get(timeout)) {
+        auto msg = std::move(*maybe_msg);
+        if (!handle_message(msg))
           return;
         ++received;
       } else {
@@ -392,7 +386,7 @@ void pong_mode(broker::endpoint& ep, topic_list topics) {
 
 // -- main function ------------------------------------------------------------
 
-int main(int argc, char** argv) {
+int main(int argc, char** argv) try {
   broker::endpoint::system_guard sys_guard; // Initialize global state.
   setvbuf(stdout, NULL, _IOLBF, 0);         // Always line-buffer stdout.
   // Parse CLI parameters using our config.
@@ -492,4 +486,7 @@ int main(int argc, char** argv) {
   }
   // Stop utility actors.
   anon_send_exit(verbose_logger, exit_reason::user_shutdown);
+} catch (std::exception& ex) {
+  std::cerr << "*** exception: " << ex.what() << "\n";
+  return EXIT_FAILURE;
 }

src/broker-pipe.cc

@@ -123,7 +123,7 @@ void publish_mode_select(broker::endpoint& ep, const std::string& topic_str,
       if (!std::getline(std::cin, line))
         return; // Reached end of STDIO.
       else
-        out.publish(std::move(line));
+        out.publish(line);
     i += num;
     msg_count += num;
   }
@@ -215,7 +215,7 @@ void split(std::vector<std::string>& result, std::string_view str,
 
 } // namespace
 
-int main(int argc, char** argv) {
+int main(int argc, char** argv) try {
   broker::endpoint::system_guard sys_guard;
   // Parse CLI parameters using our config.
   parameters params;
@@ -301,4 +301,7 @@ int main(int argc, char** argv) {
   auto i = std::find(b, std::end(as), std::make_pair(params.mode, params.impl));
   auto f = fs[std::distance(b, i)];
   f(ep, params.topic, params.message_cap);
+} catch (std::exception& ex) {
+  std::cerr << "*** exception: " << ex.what() << "\n";
+  return EXIT_FAILURE;
 }

src/data.cc

@@ -94,8 +94,8 @@ const char* data::get_type_name() const {
 namespace {
 
 template <class Container>
-void container_convert(Container& c, std::string& str, const char* left,
-                       const char* right, const char* delim = ", ") {
+void container_convert(Container& c, std::string& str, char left, char right) {
+  constexpr auto* delim = ", ";
   auto first = begin(c);
   auto last = end(c);
   str += left;
@@ -141,15 +141,15 @@ void convert(const table::value_type& x, std::string& str) {
 }
 
 void convert(const vector& x, std::string& str) {
-  container_convert(x, str, "(", ")");
+  container_convert(x, str, '(', ')');
 }
 
 void convert(const set& x, std::string& str) {
-  container_convert(x, str, "{", "}");
+  container_convert(x, str, '{', '}');
 }
 
 void convert(const table& x, std::string& str) {
-  container_convert(x, str, "{", "}");
+  container_convert(x, str, '{', '}');
 }
 
 void convert(const data& x, std::string& str) {

src/detail/peer_status_map.cc

@@ -30,37 +30,37 @@ bool peer_status_map::update(endpoint_id peer, peer_status& expected,
   if (closed_) {
     expected = peer_status::unknown;
     return false;
-  } else if (auto i = peers_.find(peer); i != peers_.end()) {
+  }
+  if (auto i = peers_.find(peer); i != peers_.end()) {
     if (i->second == expected) {
       i->second = desired;
       return true;
     } else {
       expected = i->second;
       return false;
     }
-  } else {
-    expected = peer_status::unknown;
-    return false;
   }
+  expected = peer_status::unknown;
+  return false;
 }
 
 bool peer_status_map::remove(endpoint_id peer, peer_status& expected) {
   std::unique_lock guard{mtx_};
   if (closed_) {
     expected = peer_status::unknown;
     return false;
-  } else if (auto i = peers_.find(peer); i != peers_.end()) {
+  }
+  if (auto i = peers_.find(peer); i != peers_.end()) {
     if (i->second == expected) {
       peers_.erase(i);
       return true;
     } else {
       expected = i->second;
       return false;
     }
-  } else {
-    expected = peer_status::unknown;
-    return false;
   }
+  expected = peer_status::unknown;
+  return false;
 }
 
 void peer_status_map::remove(endpoint_id peer) {

src/detail/prefix_matcher.cc

@@ -4,7 +4,7 @@ namespace broker {
 namespace detail {
 
 bool prefix_matcher::operator()(const filter_type& filter,
-                                const topic& t) const {
+                                const topic& t) const noexcept {
   for (auto& prefix : filter)
     if (prefix.prefix_of(t))
       return true;

src/endpoint.cc

@@ -641,7 +641,6 @@ uint16_t endpoint::listen(const std::string& address, uint16_t port,
   BROKER_INFO("try listening on"
               << (address + ":" + std::to_string(port))
               << (ctx_->cfg.options().disable_ssl ? "(no SSL)" : "(SSL)"));
-  char const* addr = address.empty() ? nullptr : address.c_str();
   uint16_t result = 0;
   caf::scoped_actor self{ctx_->sys};
   self
@@ -844,7 +843,9 @@ worker endpoint::do_subscribe(filter_type&& filter,
   BROKER_ASSERT(sink != nullptr);
   using caf::async::make_spsc_buffer_resource;
   // Get a pair of connected resources.
-  auto [con_res, prod_res] = make_spsc_buffer_resource<data_message>();
+  // Note: structured bindings with values confuses clang-tidy's leak checker.
+  auto resources = make_spsc_buffer_resource<data_message>();
+  auto& [con_res, prod_res] = resources;
   // Subscribe a new worker to the consumer end.
   auto [obs, launch_obs] = ctx_->sys.spawn_inactive<worker_actor>();
   sink->init();
@@ -916,7 +917,9 @@ worker endpoint::do_publish_all(std::shared_ptr<detail::source_driver> driver) {
   BROKER_ASSERT(driver != nullptr);
   using caf::async::make_spsc_buffer_resource;
   // Get a pair of connected resources.
-  auto [con_res, prod_res] = make_spsc_buffer_resource<data_message>();
+  // Note: structured bindings with values confuses clang-tidy's leak checker.
+  auto resources = make_spsc_buffer_resource<data_message>();
+  auto [con_res, prod_res] = resources;
   // Push to the producer end with a new worker.
   auto [src, launch_src] = ctx_->sys.spawn_inactive<worker_actor>();
   driver->init();

src/error.cc

@@ -105,7 +105,8 @@ error::error(error&& other) noexcept {
 }
 
 error& error::operator=(const error& other) {
-  native(*this) = native(other);
+  if (this != &other)
+    native(*this) = native(other);
   return *this;
 }
 
@@ -306,12 +307,13 @@ const std::string* error_view::message() const noexcept {
 }
 
 std::optional<endpoint_info> error_view::context() const {
-  if (is<none>((*xs_)[2]))
+  if (is<none>((*xs_)[2])) {
     return std::nullopt;
-  else if (auto& ctx = get<vector>((*xs_)[2]); ctx.size() == 2)
+  }
+  if (auto& ctx = get<vector>((*xs_)[2]); ctx.size() == 2) {
     return get_as<endpoint_info>(ctx[0]);
-  else
-    return std::nullopt;
+  }
+  return std::nullopt;
 }
 
 error_view error_view::make(const data& src) {

src/internal/connector.cc

@@ -843,7 +843,6 @@ class connect_state : public std::enable_shared_from_this<connect_state> {
             return read_result::stop;
           payload_size = 0;
           read_pos = 0;
-          read_size = 4;
           return reached_fin_state() ? read_result::stop : read_result::again;
         } else {
           return read_result::again;
@@ -1012,7 +1011,7 @@ struct connect_manager {
                    << (event == read_mask ? "reading" : "writing")
                    << BROKER_ARG2("fd", i->first));
       if (auto fds_ptr = find_pollfd(i->first)) {
-        fds_ptr->events |= event;
+        fds_ptr->events = static_cast<short>(fds_ptr->events | event);
       } else {
         pending_fdset.emplace_back(pollfd{i->first, event, 0});
       }
@@ -1753,7 +1752,8 @@ connector::connector(endpoint_id this_peer, broker_options broker_cfg,
               static_cast<int>(max_ssl_passphrase_size));
       ::abort();
     }
-    strcpy(ssl_passphrase_buf, ssl_cfg_->passphrase.c_str());
+    strncpy(ssl_passphrase_buf, ssl_cfg_->passphrase.c_str(),
+            max_ssl_passphrase_size);
   }
 }