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/error.hh

@@ -183,7 +183,7 @@ public:
   }
 
 private:
-  std::byte obj_[sizeof(impl*)];
+  std::byte obj_[sizeof(void*)];
 };
 
 /// @relates error
@@ -350,7 +350,7 @@ inline error_view make_error_view(const data& src) {
 } // namespace broker
 
 #define BROKER_TRY_IMPL(statement)                                             \
-  if (auto err = statement)                                                    \
+  if (auto err = (statement))                                                  \
   return err
 
 #define BROKER_TRY_1(x1) BROKER_TRY_IMPL(x1)

include/broker/internal/type_id.hh

@@ -15,9 +15,11 @@
 
 // -- imported atoms -----------------------------------------------------------
 
+// NOLINTBEGIN
 #define BROKER_CAF_ATOM_ALIAS(name)                                            \
   using name = caf::name##_atom;                                               \
   constexpr auto name##_v = caf::name##_atom_v;
+// NOLINTEND
 
 namespace broker::internal::atom {
 
@@ -50,11 +52,12 @@ static_assert(caf::has_type_id_v<broker::timespan>,
 static_assert(caf::has_type_id_v<broker::timestamp>,
               "broker::timestamp != caf::timestamp");
 
-#define BROKER_ADD_ATOM(name) CAF_ADD_ATOM(broker, broker::internal::atom, name)
+#define BROKER_ADD_ATOM(name)                                                  \
+  CAF_ADD_ATOM(broker_internal, broker::internal::atom, name)
 
-#define BROKER_ADD_TYPE_ID(type) CAF_ADD_TYPE_ID(broker, type)
+#define BROKER_ADD_TYPE_ID(type) CAF_ADD_TYPE_ID(broker_internal, type)
 
-CAF_BEGIN_TYPE_ID_BLOCK(broker, caf::first_custom_type_id)
+CAF_BEGIN_TYPE_ID_BLOCK(broker_internal, caf::first_custom_type_id)
 
   // -- atoms for generic communication ----------------------------------------
 
@@ -86,6 +89,7 @@ CAF_BEGIN_TYPE_ID_BLOCK(broker, caf::first_custom_type_id)
   BROKER_ADD_ATOM(await)
   BROKER_ADD_ATOM(clear)
   BROKER_ADD_ATOM(clone)
+  BROKER_ADD_ATOM(data_store)
   BROKER_ADD_ATOM(decrement)
   BROKER_ADD_ATOM(erase)
   BROKER_ADD_ATOM(exists)
@@ -99,7 +103,6 @@ CAF_BEGIN_TYPE_ID_BLOCK(broker, caf::first_custom_type_id)
   BROKER_ADD_ATOM(resolve)
   BROKER_ADD_ATOM(restart)
   BROKER_ADD_ATOM(stale_check)
-  BROKER_ADD_ATOM(store)
   BROKER_ADD_ATOM(subtract)
   BROKER_ADD_ATOM(sync_point)
 
@@ -173,7 +176,7 @@ CAF_BEGIN_TYPE_ID_BLOCK(broker, caf::first_custom_type_id)
   BROKER_ADD_TYPE_ID((std::shared_ptr<std::promise<void>>) )
   BROKER_ADD_TYPE_ID((std::vector<broker::peer_info>) )
 
-CAF_END_TYPE_ID_BLOCK(broker)
+CAF_END_TYPE_ID_BLOCK(broker_internal)
 
 // -- enable opt-in features for Broker types ----------------------------------
 

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/store_event.hh

@@ -73,7 +73,7 @@ public:
         return {};
     }
 
-    entity_id publisher() const noexcept {
+    entity_id publisher() const {
       if (auto value = to<endpoint_id>((*xs_)[5])) {
         return {std::move(*value), get<uint64_t>((*xs_)[6])};
       }
@@ -119,7 +119,7 @@ public:
       return xs_ != nullptr;
     }
 
-    const std::string& store_id() const noexcept {
+    const std::string& store_id() const {
       return get<std::string>((*xs_)[1]);
     }
 
@@ -142,7 +142,7 @@ public:
         return {};
     }
 
-    entity_id publisher() const noexcept {
+    entity_id publisher() const {
       if (auto value = to<endpoint_id>((*xs_)[6]))
         return {*value, get<uint64_t>((*xs_)[7])};
       else
@@ -186,15 +186,15 @@ public:
       return xs_ != nullptr;
     }
 
-    const std::string& store_id() const noexcept {
+    const std::string& store_id() const {
       return get<std::string>((*xs_)[1]);
     }
 
     const data& key() const noexcept {
       return (*xs_)[2];
     }
 
-    entity_id publisher() const noexcept {
+    entity_id publisher() const {
       if (auto value = to<endpoint_id>((*xs_)[3])) {
         return {*value, get<uint64_t>((*xs_)[4])};
       }
@@ -238,15 +238,15 @@ public:
       return xs_ != nullptr;
     }
 
-    const std::string& store_id() const noexcept {
+    const std::string& store_id() const {
       return get<std::string>((*xs_)[1]);
     }
 
     const data& key() const noexcept {
       return (*xs_)[2];
     }
 
-    entity_id publisher() const noexcept {
+    entity_id publisher() const {
       if (auto value = to<endpoint_id>((*xs_)[3]))
         return {*value, get<uint64_t>((*xs_)[4])};
       else

include/broker/worker.hh

@@ -4,6 +4,7 @@
 #include "broker/detail/comparable.hh"
 
 #include <cstddef>
+#include <cstdint>
 #include <functional>
 #include <string>
 #include <utility>
@@ -20,9 +21,9 @@ public:
   // -- constants --------------------------------------------------------------
 
 #ifdef BROKER_WINDOWS
-  static constexpr size_t obj_size = sizeof(impl*) * 2;
+  static constexpr size_t obj_size = sizeof(void*) * 2;
 #else
-  static constexpr size_t obj_size = sizeof(impl*);
+  static constexpr size_t obj_size = sizeof(void*);
 #endif
 
   // --- construction and destruction ------------------------------------------
@@ -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

@@ -54,7 +54,7 @@ namespace atom = broker::internal::atom;
 #define BROKER_NODE_ADD_ATOM(name, text)                                       \
   CAF_ADD_ATOM(broker_node, broker::atom, name, text)
 
-CAF_BEGIN_TYPE_ID_BLOCK(broker_node, id_block::broker::end)
+CAF_BEGIN_TYPE_ID_BLOCK(broker_node, id_block::broker_internal::end)
 
   BROKER_NODE_ADD_ATOM(blocking, "blocking")
   BROKER_NODE_ADD_ATOM(relay, "relay")
@@ -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/configuration.cc

@@ -515,7 +515,7 @@ std::once_flag init_global_state_flag;
 
 void configuration::init_global_state() {
   std::call_once(init_global_state_flag, [] {
-    caf::init_global_meta_objects<caf::id_block::broker>();
+    caf::init_global_meta_objects<caf::id_block::broker_internal>();
     caf::io::middleman::init_global_meta_objects();
     caf::core::init_global_meta_objects();
   });