Fix clang-tidy findings and re-enable on CI

.cirrus.yml

@@ -56,7 +56,7 @@ unix_env: &UNIX_ENV
 
 clang_tidy_task:
   container:
-    dockerfile: ci/debian-12/Dockerfile
+    dockerfile: ci/debian-13/Dockerfile
     << : *RESOURCES_TEMPLATE
   sync_submodules_script: git submodule update --recursive --init
   build_script: ./ci/analyze.sh
@@ -95,10 +95,10 @@ debian12_task:
   << : *CI_TEMPLATE
   << : *UNIX_ENV
 
-debian11_task:
+debian13_task:
   container:
-    # Debian 11 EOL: June 2026
-    dockerfile: ci/debian-11/Dockerfile
+    # Debian 13 EOL: TBD
+    dockerfile: ci/debian-13/Dockerfile
     << : *RESOURCES_TEMPLATE
   << : *CI_TEMPLATE
   << : *UNIX_ENV
@@ -192,7 +192,7 @@ freebsd13_task:
 memcheck_task:
   container:
     # Just uses a recent/common distro to run memory error/leak checks.
-    dockerfile: ci/debian-11/Dockerfile
+    dockerfile: ci/debian-13/Dockerfile
     cpu: 8
     # AddressSanitizer uses more memory than normal config.
     memory: 8GB

.clang-tidy

@@ -6,9 +6,12 @@
           -modernize-use-ranges,
           performance-*,
           -performance-enum-size,
+          -performance-unnecessary-value-param,
           -bugprone-easily-swappable-parameters,
           -bugprone-forward-declaration-namespace,
           -bugprone-exception-escape,
+          -bugprone-macro-parentheses,
+          -clang-analyzer-core.StackAddressEscape,
           -clang-analyzer-cplusplus.NewDeleteLeaks,
           -clang-analyzer-security.insecureAPI.rand,
           -clang-analyzer-webkit*,

ci/analyze.sh

@@ -18,4 +18,4 @@ fi
 
 CXX=clang++ ./configure --build-type=debug
 clang-tidy --version
-run-clang-tidy -p build -j ${BROKER_CI_CPUS} $PWD/src
+run-clang-tidy -p build -j ${BROKER_CI_CPUS} $PWD/libbroker

ci/debian-11/Dockerfile → ci/debian-13/Dockerfile

@@ -1,4 +1,4 @@
-FROM debian:11
+FROM debian:13
 
 ENV DEBIAN_FRONTEND="noninteractive" TZ="America/Los_Angeles"
 
@@ -7,6 +7,8 @@ ENV DEBIAN_FRONTEND="noninteractive" TZ="America/Los_Angeles"
 ENV DOCKERFILE_VERSION=20230813
 
 RUN apt-get update && apt-get -y install \
+    clang \
+    clang-tidy \
     cmake \
     curl \
     g++ \

libbroker/broker/address.cc

@@ -87,21 +87,26 @@ address& address::operator=(const address& other) noexcept {
   return *this;
 }
 
-static uint32_t bit_mask32(int bottom_bits) {
+namespace {
+
+uint32_t bit_mask32(unsigned bottom_bits) {
   if (bottom_bits >= 32)
     return 0xffffffff;
   return (((uint32_t) 1) << bottom_bits) - 1;
 }
 
+} // namespace
+
 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::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));
-  for (auto i = res.quot + 1; i < 4; ++i)
+  unsigned res_quot = top_bits_to_keep / 32u;
+  unsigned res_rem = top_bits_to_keep % 32u;
+  if (res_quot < 4)
+    mask[res_quot] = caf::detail::to_network_order(
+      mask[res_quot] & ~bit_mask32(32u - res_rem));
+  for (auto i = res_quot + 1; i < 4; ++i)
     mask[i] = 0;
   auto p = reinterpret_cast<uint32_t*>(&bytes());
   for (auto i = 0; i < 4; ++i)

libbroker/broker/alm/multipath.test.cc

@@ -85,7 +85,6 @@ TEST(multipath trees grow with emplace) {
   CHECK_EQUAL(ab->head().id(), ids['B']);
   emplace(ab, 'F');
   emplace(ab, 'G');
-  std::string buf;
   auto path = multipath{tptr};
   CHECK_EQUAL(stringify(path), "(A, [(B, [(F), (G)]), (C, [(D), (E)])])");
 }
@@ -112,6 +111,8 @@ TEST(multipaths are copy constructible and comparable) {
   auto path2 = path1;
   CHECK_EQUAL(stringify(path1), stringify(path2));
   CHECK_EQUAL(path1, path2);
+  path1 = multipath{};
+  CHECK_NOT_EQUAL(path1, path2);
 }
 
 TEST(multipaths are serializable) {
@@ -164,8 +165,8 @@ TEST(source routing extracts multipaths from routing tables) {
   //
   alm::routing_table tbl;
   auto add = [&](char id, const std::vector<std::vector<char>>& paths) {
-    auto& entry = tbl.emplace(ids[id], alm::routing_table_row{}).first->second;
-    for (auto& path : paths) {
+    tbl.emplace(ids[id], alm::routing_table_row{});
+    for (const auto& path : paths) {
       std::vector<endpoint_id> xs;
       xs.reserve(path.size());
       for (auto c : path)

libbroker/broker/alm/routing_table.test.cc

@@ -49,8 +49,8 @@ struct fixture : ids_fixture {
     //
     auto add = [&](endpoint_id id,
                    const std::vector<std::vector<endpoint_id>>& paths) {
-      auto& entry = tbl.emplace(id, alm::routing_table_row{}).first->second;
-      for (auto& path : paths)
+      tbl.emplace(id, alm::routing_table_row{});
+      for (const auto& path : paths)
         add_or_update_path(tbl, id, path, vector_timestamp(path.size()));
     };
     add(B, {{B}, {J, I, D, B}, {J, I, E, B}});

libbroker/broker/builder.hh

@@ -399,7 +399,7 @@ protected:
     using val_t = std::decay_t<decltype(pval)>;
     if constexpr (detail::is_tuple<val_t>) {
       auto out = format::bin::v1::write_unsigned(data::type::vector, adder());
-      format::bin::v1::write_varbyte(std::tuple_size<val_t>::value, out);
+      format::bin::v1::write_varbyte(std::tuple_size_v<val_t>, out);
       std::apply([&](auto&&... xs) { (add_inline_vector_item(xs), ...); },
                  pval);
     } else {

libbroker/broker/data.test.cc

@@ -21,6 +21,11 @@ static_assert(std::is_same_v<integer, int64_t>);
 static_assert(std::is_same_v<count, uint64_t>);
 static_assert(std::is_same_v<real, double>);
 
+// Note: the `CHECK(opt) && opt` pattern exists only because clang-tidy doesn't
+//       recognize that `CHECK(opt)` already checks that `opt` contains a value.
+//       The `&& opt` is completely redundant, but it suppresses the false
+//       positives.
+
 TEST(timespan) {
   auto s = timespan{42};
   CHECK(std::chrono::nanoseconds{42} == s);
@@ -42,19 +47,19 @@ TEST(address) {
   CHECK(!a.is_v4());
   CHECK(a.is_v6());
   MESSAGE("parsing");
-  auto opt = to<address>("dead::beef");
-  REQUIRE(opt);
-  CHECK(!opt->is_v4());
-  CHECK(opt->is_v6());
-  opt = to<address>("1.2.3.4");
-  REQUIRE(opt);
-  CHECK(opt->is_v4());
-  CHECK(!opt->is_v6());
-  MESSAGE("printing");
-  CHECK_EQUAL(to_string(*opt), "1.2.3.4");
-  MESSAGE("masking");
-  CHECK(opt->mask(96 + 16));
-  CHECK_EQUAL(to_string(*opt), "1.2.0.0");
+  if (auto opt = to<address>("dead::beef"); CHECK(opt) && opt) {
+    CHECK(!opt->is_v4());
+    CHECK(opt->is_v6());
+  }
+  if (auto opt = to<address>("1.2.3.4"); CHECK(opt) && opt) {
+    CHECK(opt->is_v4());
+    CHECK(!opt->is_v6());
+    MESSAGE("printing");
+    CHECK_EQUAL(to_string(*opt), "1.2.3.4");
+    MESSAGE("masking");
+    CHECK(opt->mask(96 + 16));
+    CHECK_EQUAL(to_string(*opt), "1.2.0.0");
+  }
 }
 
 TEST(port) {
@@ -63,14 +68,14 @@ TEST(port) {
   CHECK(p.type() == port::protocol::unknown);
   p = {80, port::protocol::tcp};
   MESSAGE("parsing");
-  auto opt = to<port>("8/icmp");
-  REQUIRE(opt);
-  CHECK_EQUAL(*opt, port(8, port::protocol::icmp));
-  opt = to<port>("42/nonsense");
-  REQUIRE(opt);
-  CHECK_EQUAL(*opt, port(42, port::protocol::unknown));
-  MESSAGE("printing");
-  CHECK_EQUAL(to_string(p), "80/tcp");
+  if (auto opt = to<port>("8/icmp"); CHECK(opt) && opt) {
+    CHECK_EQUAL(*opt, port(8, port::protocol::icmp));
+  }
+  if (auto opt = to<port>("42/nonsense"); CHECK(opt) && opt) {
+    CHECK_EQUAL(*opt, port(42, port::protocol::unknown));
+    MESSAGE("printing");
+    CHECK_EQUAL(to_string(p), "80/tcp");
+  }
   p = {0, port::protocol::unknown};
   CHECK_EQUAL(to_string(p), "0/?");
 }
@@ -81,11 +86,11 @@ TEST(subnet) {
   CHECK_EQUAL(to_string(sn), "::/0");
   auto a = to<address>("1.2.3.4");
   auto b = to<address>("1.2.3.0");
-  REQUIRE(a);
-  REQUIRE(b);
-  sn = {*a, 24};
-  CHECK_EQUAL(sn.length(), 24u);
-  CHECK_EQUAL(sn.network(), *b);
+  if (CHECK(a && b) && a && b) {
+    sn = {*a, 24};
+    CHECK_EQUAL(sn.length(), 24u);
+    CHECK_EQUAL(sn.network(), *b);
+  }
 }
 
 TEST(data - construction) {

libbroker/broker/error.test.cc

@@ -118,12 +118,14 @@ TEST(error view operate directly on raw data) {
   CHECK_EQUAL(view.code(), ec::peer_invalid);
   CHECK_EQUAL(*view.message(), "invalid host"s);
   auto maybe_cxt = view.context();
-  REQUIRE(maybe_cxt);
-  auto cxt = std::move(*maybe_cxt);
-  CHECK_EQUAL(cxt.node, ids['B']);
-  REQUIRE(cxt.network);
-  auto net = *cxt.network;
-  CHECK_EQUAL(net, network_info("foo", 8080, timeout::seconds{42}));
+  if (CHECK(maybe_cxt) && maybe_cxt) {
+    auto cxt = std::move(*maybe_cxt);
+    CHECK_EQUAL(cxt.node, ids['B']);
+    auto& net = cxt.network;
+    if (CHECK(net) && net) {
+      CHECK_EQUAL(*net, network_info("foo", 8080, timeout::seconds{42}));
+    }
+  }
 }
 
 FIXTURE_SCOPE_END()

libbroker/broker/expected.hh

@@ -200,6 +200,11 @@ public:
     return engaged_;
   }
 
+  /// Returns `true` if the object holds a value (is engaged).
+  bool has_value() const noexcept {
+    return engaged_;
+  }
+
   /// Returns the contained error.
   /// @pre `engaged() == false`.
   const broker::error& cerror() const noexcept {

libbroker/broker/hub.test.cc

@@ -88,15 +88,16 @@ TEST("subscribers receive messages from hubs but not from publishers") {
   auto uut = ep.make_hub({"/foo/bar"});
   auto sub = ep.make_subscriber({"/foo/bar"});
   uut.publish("/foo/bar", broker::list_builder{}.add(1).add(2).add(3));
-  if (auto msg = sub.get(150ms); CHECK(msg.has_value())) {
+  if (auto msg = sub.get(150ms); CHECK(msg) && msg) {
     CHECK_EQ((*msg)->topic(), "/foo/bar");
     if (auto val = (*msg)->value(); CHECK(val.is_list())) {
       CHECK_EQ(broker::to_string(val), "(1, 2, 3)");
     }
   }
   auto pub = ep.make_publisher(broker::topic{"/foo/bar"});
   pub.publish(broker::list_builder{}.add(4).add(5).add(6));
-  if (auto msg = sub.get(150ms); !CHECK(!msg.has_value())) {
+  if (auto msg = sub.get(150ms)) {
     MESSAGE("unexpected message: " << broker::to_string((*msg)->value()));
+    CHECK(false);
   }
 }

libbroker/broker/internal/channel.test.cc

@@ -168,10 +168,12 @@ struct fixture {
     std::shuffle(outgoing_messages.begin(), outgoing_messages.end(), rng);
     if (loss_rate > 0) {
       auto num_message = outgoing_messages.size();
-      auto lost = static_cast<size_t>(ceil(num_message * loss_rate));
+      auto lost =
+        static_cast<size_t>(ceil(static_cast<double>(num_message) * loss_rate));
       assert(num_message >= lost);
       auto new_size = std::max(num_message - lost, size_t{1});
-      auto i = outgoing_messages.begin() + new_size;
+      auto i = outgoing_messages.begin()
+               + static_cast<std::ptrdiff_t>(new_size);
       outgoing_messages.erase(i, outgoing_messages.end());
     }
   }

libbroker/broker/internal/type_id.hh

@@ -16,11 +16,9 @@
 
 // -- 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 {
 

libbroker/broker/logger.cc

@@ -87,7 +87,7 @@ class console_logger : public event_observer {
       msecs_str.insert(msecs_str.begin(), 1, '0');
     }
     // Critical section to avoid interleaved output.
-    std::lock_guard guard{mtx_};
+    std::scoped_lock guard{mtx_};
     std::cout << severity_color(what->severity)
               << std::put_time(std::localtime(&secs), "%F %T") << '.'
               << msecs_str << ' ' << severity_name(what->severity) << ' '

libbroker/broker/peering.test.cc

@@ -24,7 +24,7 @@
   })()
 
 #define SYNC_CHECK_FAILED(...)                                                 \
-  ([](auto... xs) {                                                            \
+  ([](const auto&... xs) {                                                     \
     std::ostringstream str;                                                    \
     (str << ... << xs);                                                        \
     std::unique_lock guard{print_mtx};                                         \

libbroker/broker/status.test.cc

@@ -93,11 +93,14 @@ TEST(status views operate directly on raw data) {
   CHECK_EQUAL(view.code(), sc::peer_added);
   CHECK_EQUAL(*view.message(), "text"s);
   auto cxt = view.context();
-  REQUIRE(cxt);
-  REQUIRE(cxt->network);
-  CHECK_EQUAL(cxt->node, id);
-  CHECK_EQUAL(cxt->network->address, "foo");
-  CHECK_EQUAL(cxt->network->port, 8080u);
+  if (CHECK(cxt) && cxt) {
+    CHECK_EQUAL(cxt->node, id);
+    auto& net = cxt->network;
+    if (CHECK(net) && net) {
+      CHECK_EQUAL(net->address, "foo");
+      CHECK_EQUAL(net->port, 8080u);
+    }
+  }
 }
 
 FIXTURE_SCOPE_END()

libbroker/broker/topic.hh

@@ -103,9 +103,11 @@ public:
     return f.apply(x.str_);
   }
 
+  // NOLINTBEGIN(clang-analyzer-cplusplus.Move)
   auto compare(const topic& other) const {
     return str_.compare(other.str_);
   }
+  // NOLINTEND(clang-analyzer-cplusplus.Move)
 
 private:
   static topic from(std::string_view str) {