chore: Fix all clang build warnings (#4475)

* chore: Fix all clang build warnings

Also add `-Werror` to clang build in CI.

Fixes #4449

* all build targets

* fix search test

.github/workflows/ci.yml

@@ -48,7 +48,6 @@ jobs:
           - container: "alpine-dev:latest"
             build-type: Debug
             compiler: { cxx: clang++, c: clang }
-            cxx_flags: ""
 
     runs-on: ubuntu-latest
     env:

helio

@@ -1 +1 @@
-Subproject commit 90bc3cc6aabbffc8b274dc0f7801695d68658529
+Subproject commit 05c316e171e31a6f23165b426b7caf0174f90838

src/core/dash_internal.h

@@ -20,10 +20,10 @@ namespace detail {
 
 template <unsigned NUM_SLOTS> class SlotBitmap {
   static_assert(NUM_SLOTS > 0 && NUM_SLOTS <= 28);
-  static constexpr unsigned kLen = NUM_SLOTS > 14 ? 2 : 1;
+  static constexpr bool SINGLE = NUM_SLOTS <= 14;
+  static constexpr unsigned kLen = SINGLE ? 1 : 2;
   static constexpr unsigned kAllocMask = (1u << NUM_SLOTS) - 1;
   static constexpr unsigned kBitmapLenMask = (1 << 4) - 1;
-  static constexpr bool SINGLE = NUM_SLOTS <= 14;
 
  public:
   // probe - true means the entry is probing, i.e. not owning.
@@ -32,7 +32,8 @@ template <unsigned NUM_SLOTS> class SlotBitmap {
   uint32_t GetProbe(bool probe) const {
     if constexpr (SINGLE)
       return ((val_[0].d >> 4) & kAllocMask) ^ ((!probe) * kAllocMask);
-    return (val_[1].d & kAllocMask) ^ ((!probe) * kAllocMask);
+    else
+      return (val_[1].d & kAllocMask) ^ ((!probe) * kAllocMask);
   }
 
   // GetBusy returns the busy mask.

src/core/json/jsonpath_grammar.y

@@ -26,10 +26,6 @@
 #include "src/core/json/lexer_impl.h"
 #include "src/core/json/driver.h"
 
-// Have to disable because GCC doesn't understand `symbol_type`'s union
-// implementation
-#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
-
 #define yylex driver->lexer()->Lex
 
 using namespace std;

src/core/json/jsonpath_test.cc

@@ -465,7 +465,7 @@ TYPED_TEST(JsonPathTest, Mutate) {
   Path path = this->driver_.TakePath();
 
   TypeParam json = ValidJson<TypeParam>(R"([1, 2, 3, 5, 6])");
-  MutateCallback cb = [&](optional<string_view>, JsonType* val) {
+  auto cb = [](optional<string_view>, JsonType* val) {
     int intval = val->as<int>();
     *val = intval + 1;
     return false;
@@ -496,7 +496,7 @@ TYPED_TEST(JsonPathTest, Mutate) {
   ASSERT_EQ(0, this->Parse("$..a.*"));
   path = this->driver_.TakePath();
 
-  MutateCallback cb2 = [&](optional<string_view> key, JsonType* val) {
+  auto cb2 = [](optional<string_view> key, JsonType* val) {
     if (val->is_int64() && !key) {  // array element
       *val = 42;
       return false;

src/core/search/parser.y

@@ -27,10 +27,6 @@
 #include "core/search/query_driver.h"
 #include "core/search/vector_utils.h"
 
-// Have to disable because GCC doesn't understand `symbol_type`'s union
-// implementation
-#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
-
 #define yylex driver->scanner()->Lex
 
 using namespace std;
@@ -196,12 +192,12 @@ dfly::search::Parser::error(const location_type& l, const string& m)
 
 std::uint32_t toUint32(string_view str) {
   uint32_t val = 0;
-  absl::SimpleAtoi(str, &val); // no need to check the result because str is parsed by regex
+  std::ignore = absl::SimpleAtoi(str, &val); // no need to check the result because str is parsed by regex
   return val;
 }
 
 double toDouble(string_view str) {
   double val = 0;
-  absl::SimpleAtod(str, &val); // no need to check the result because str is parsed by regex
+  std::ignore = absl::SimpleAtod(str, &val); // no need to check the result because str is parsed by regex
   return val;
 }

src/core/search/vector_utils.cc

@@ -15,9 +15,14 @@ using namespace std;
 
 namespace {
 
+#if defined(__GNUC__) && !defined(__clang__)
+#define FAST_MATH __attribute__((optimize("fast-math")))
+#else
+#define FAST_MATH
+#endif
+
 // Euclidean vector distance: sqrt( sum: (u[i] - v[i])^2  )
-__attribute__((optimize("fast-math"))) float L2Distance(const float* u, const float* v,
+FAST_MATH float L2Distance(const float* u, const float* v, size_t dims) {
-                                                        size_t dims) {
   float sum = 0;
   for (size_t i = 0; i < dims; i++)
     sum += (u[i] - v[i]) * (u[i] - v[i]);
@@ -25,8 +30,7 @@ __attribute__((optimize("fast-math"))) float L2Distance(const float* u, const fl
 }
 
 // TODO: Normalize vectors ahead if cosine distance is used
-__attribute__((optimize("fast-math"))) float CosineDistance(const float* u, const float* v,
+FAST_MATH float CosineDistance(const float* u, const float* v, size_t dims) {
-                                                            size_t dims) {
   float sum_uv = 0, sum_uu = 0, sum_vv = 0;
   for (size_t i = 0; i < dims; i++) {
     sum_uv += u[i] * v[i];

src/core/task_queue.cc

@@ -14,7 +14,7 @@ namespace dfly {
 __thread unsigned TaskQueue::blocked_submitters_ = 0;
 
 TaskQueue::TaskQueue(unsigned queue_size, unsigned start_size, unsigned pool_max_size)
-    : queue_(queue_size), consumer_fibers_(start_size), pool_max_size_(pool_max_size) {
+    : queue_(queue_size), consumer_fibers_(start_size) {
   CHECK_GT(start_size, 0u);
   CHECK_LE(start_size, pool_max_size);
 }

src/core/task_queue.h

@@ -66,7 +66,6 @@ class TaskQueue {
  private:
   util::fb2::FiberQueue queue_;
   std::vector<util::fb2::Fiber> consumer_fibers_;
-  unsigned pool_max_size_;
 
   static __thread unsigned blocked_submitters_;
 };

src/server/acl/validator.cc

@@ -43,12 +43,6 @@ namespace dfly::acl {
   return is_authed;
 }
 
-// GCC yields a wrong warning about uninitialized optional use
-#ifdef __GNUC__
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
-#endif
-
 static bool ValidateCommand(const std::vector<uint64_t>& acl_commands, const CommandId& id) {
   const size_t index = id.GetFamily();
   const uint64_t command_mask = id.GetBitIndex();
@@ -130,6 +124,4 @@ static bool ValidateCommand(const std::vector<uint64_t>& acl_commands, const Com
   return {allowed, AclLog::Reason::PUB_SUB};
 }
 
-#pragma GCC diagnostic pop
-
 }  // namespace dfly::acl

src/server/bitops_family.cc

@@ -581,7 +581,7 @@ void BitCount(CmdArgList args, const CommandContext& cmd_cntx) {
   if (!parser.Finalize()) {
     return builder->SendError(parser.Error()->MakeReply());
   }
-  auto cb = [&](Transaction* t, EngineShard* shard) {
+  auto cb = [&, &start = start, &end = end](Transaction* t, EngineShard* shard) {
     return CountBitsForValue(t->GetOpArgs(shard), key, start, end, as_bit);
   };
   OpResult<std::size_t> res = cmd_cntx.tx->ScheduleSingleHopT(std::move(cb));
@@ -1225,7 +1225,7 @@ void SetBit(CmdArgList args, const CommandContext& cmd_cntx) {
     return cmd_cntx.rb->SendError(err->MakeReply());
   }
 
-  auto cb = [&](Transaction* t, EngineShard* shard) {
+  auto cb = [&, &key = key, &offset = offset, &value = value](Transaction* t, EngineShard* shard) {
     return BitNewValue(t->GetOpArgs(shard), key, offset, value != 0);
   };
 

src/server/detail/snapshot_storage.cc

@@ -334,7 +334,8 @@ io::Result<vector<string>, GenericError> GcsSnapshotStorage::ExpandFromPath(
 
   // Find snapshot shard files if we're loading DFS.
   fb2::ProactorBase* proactor = shard_set->pool()->GetNextProactor();
-  auto paths = proactor->Await([&]() -> io::Result<vector<string>, GenericError> {
+  auto paths = proactor->Await([&, &bucket_name =
+                                       bucket_name]() -> io::Result<vector<string>, GenericError> {
     vector<string> res;
     cloud::GCS gcs(&creds_provider_, ctx_, proactor);
 
@@ -458,7 +459,8 @@ io::Result<vector<string>, GenericError> AwsS3SnapshotStorage::ExpandFromPath(
   const size_t pos = obj_path.find_last_of('/');
   const std::string prefix = (pos == std::string_view::npos) ? "" : obj_path.substr(0, pos);
 
-  auto paths = proactor->Await([&]() -> io::Result<vector<string>, GenericError> {
+  auto paths = proactor->Await([&, &bucket_name =
+                                       bucket_name]() -> io::Result<vector<string>, GenericError> {
     const io::Result<std::vector<SnapStat>, GenericError> keys = ListObjects(bucket_name, prefix);
     if (!keys) {
       return nonstd::make_unexpected(keys.error());

src/server/dfly_bench.cc

@@ -234,8 +234,8 @@ class Driver {
   }
 
   Driver(const Driver&) = delete;
-  Driver(Driver&&) = default;
+  Driver(Driver&&) = delete;
-  Driver& operator=(Driver&&) = default;
+  Driver& operator=(Driver&&) = delete;
 
   void Connect(unsigned index, const tcp::endpoint& ep);
   void Run(uint64_t* cycle_ns, CommandGenerator* cmd_gen);
@@ -458,7 +458,7 @@ void Driver::Run(uint64_t* cycle_ns, CommandGenerator* cmd_gen) {
     ThisFiber::SleepFor(1ms);
   }
 
-  socket_->Shutdown(SHUT_RDWR);  // breaks the receive fiber.
+  std::ignore = socket_->Shutdown(SHUT_RDWR);  // breaks the receive fiber.
   receive_fb_.Join();
   std::ignore = socket_->Close();
   stats_.num_clients--;

src/server/dflycmd.cc

@@ -102,7 +102,7 @@ bool WaitReplicaFlowToCatchup(absl::Time end_time, const DflyCmd::ReplicaInfo* r
 }  // namespace
 
 void DflyCmd::ReplicaInfo::Cancel() {
-  auto lk = GetExclusiveLock();
+  util::fb2::LockGuard lk{shared_mu};
   if (replica_state == SyncState::CANCELLED) {
     return;
   }
@@ -258,7 +258,7 @@ void DflyCmd::Flow(CmdArgList args, RedisReplyBuilder* rb, ConnectionContext* cn
 
   string eof_token;
   {
-    auto lk = replica_ptr->GetExclusiveLock();
+    util::fb2::LockGuard lk{replica_ptr->shared_mu};
 
     if (replica_ptr->replica_state != SyncState::PREPARATION)
       return rb->SendError(kInvalidState);
@@ -321,7 +321,7 @@ void DflyCmd::Sync(CmdArgList args, Transaction* tx, RedisReplyBuilder* rb) {
   if (!sync_id)
     return;
 
-  auto lk = replica_ptr->GetExclusiveLock();
+  util::fb2::LockGuard lk{replica_ptr->shared_mu};
   if (!CheckReplicaStateOrReply(*replica_ptr, SyncState::PREPARATION, rb))
     return;
 
@@ -359,7 +359,7 @@ void DflyCmd::StartStable(CmdArgList args, Transaction* tx, RedisReplyBuilder* r
   if (!sync_id)
     return;
 
-  auto lk = replica_ptr->GetExclusiveLock();
+  util::fb2::LockGuard lk{replica_ptr->shared_mu};
   if (!CheckReplicaStateOrReply(*replica_ptr, SyncState::FULL_SYNC, rb))
     return;
 
@@ -415,7 +415,7 @@ void DflyCmd::TakeOver(CmdArgList args, RedisReplyBuilder* rb, ConnectionContext
     return;
 
   {
-    auto lk = replica_ptr->GetSharedLock();
+    dfly::SharedLock lk{replica_ptr->shared_mu};
     if (!CheckReplicaStateOrReply(*replica_ptr, SyncState::STABLE_SYNC, rb))
       return;
 
@@ -464,7 +464,7 @@ void DflyCmd::TakeOver(CmdArgList args, RedisReplyBuilder* rb, ConnectionContext
 
   atomic_bool catchup_success = true;
   if (*status == OpStatus::OK) {
-    auto lk = replica_ptr->GetSharedLock();
+    dfly::SharedLock lk{replica_ptr->shared_mu};
     auto cb = [replica_ptr = std::move(replica_ptr), end_time,
                &catchup_success](EngineShard* shard) {
       if (!WaitReplicaFlowToCatchup(end_time, replica_ptr.get(), shard)) {
@@ -702,7 +702,7 @@ void DflyCmd::BreakStalledFlowsInShard() {
   vector<uint32_t> deleted;
 
   for (auto [sync_id, replica_ptr] : replica_infos_) {
-    auto replica_lock = replica_ptr->GetSharedLock();
+    dfly::SharedLock replica_lock{replica_ptr->shared_mu};
 
     if (!replica_ptr->flows[sid].saver)
       continue;
@@ -771,7 +771,7 @@ void DflyCmd::GetReplicationMemoryStats(ReplicationMemoryStats* stats) const {
     util::fb2::LockGuard lk{mu_};  // prevent state changes
     auto cb = [&](EngineShard* shard) ABSL_NO_THREAD_SAFETY_ANALYSIS {
       for (const auto& [_, info] : replica_infos_) {
-        auto repl_lk = info->GetSharedLock();
+        dfly::SharedLock repl_lk{info->shared_mu};
 
         // flows should not be empty.
         DCHECK(!info->flows.empty());

src/server/dflycmd.h

@@ -111,14 +111,6 @@ class DflyCmd {
           flows{flow_count} {
     }
 
-    [[nodiscard]] auto GetExclusiveLock() ABSL_EXCLUSIVE_LOCK_FUNCTION() {
-      return util::fb2::LockGuard{shared_mu};
-    }
-
-    [[nodiscard]] auto GetSharedLock() ABSL_EXCLUSIVE_LOCK_FUNCTION() {
-      return dfly::SharedLock{shared_mu};
-    }
-
     // Transition into cancelled state, run cleanup.
     void Cancel();
 

src/server/dragonfly_test.cc

@@ -774,7 +774,7 @@ TEST_F(DflyEngineTest, MemoryUsage) {
   }
 
   for (unsigned i = 0; i < 1000; ++i) {
-    Run({"rpush", "l2", StrCat(string('a', 200), i)});
+    Run({"rpush", "l2", StrCat(string(200, 'a'), i)});
   }
   auto resp = Run({"memory", "usage", "l1"});
   EXPECT_GT(*resp.GetInt(), 8000);

src/server/json_family.cc

@@ -955,8 +955,8 @@ OpResult<long> OpDel(const OpArgs& op_args, string_view key, string_view path,
 
   if (json_path.HoldsJsonPath()) {
     const json::Path& path = json_path.AsJsonPath();
-    long deletions =
+    long deletions = json::MutatePath(
-        json::MutatePath(path, [](optional<string_view>, JsonType* val) { return true; }, json_val);
+        path, [](optional<string_view>, JsonType* val) { return true; }, json_val);
     return deletions;
   }
 
@@ -1487,7 +1487,8 @@ void JsonFamily::Set(CmdArgList args, const CommandContext& cmd_cntx) {
   if (parser.Error() || parser.HasNext())  // also clear the parser error dcheck
     return builder->SendError(kSyntaxErr);
 
-  auto cb = [&](Transaction* t, EngineShard* shard) {
+  auto cb = [&, &key = key, &path = path, &json_str = json_str](Transaction* t,
+                                                                EngineShard* shard) {
     return OpSet(t->GetOpArgs(shard), key, path, json_path, json_str, is_nx_condition,
                  is_xx_condition);
   };

src/server/list_family.cc

@@ -1248,7 +1248,7 @@ void ListFamily::LPos(CmdArgList args, const CommandContext& cmd_cntx) {
   if (auto err = parser.Error(); err)
     return rb->SendError(err->MakeReply());
 
-  auto cb = [&](Transaction* t, EngineShard* shard) {
+  auto cb = [&, &key = key, &elem = elem](Transaction* t, EngineShard* shard) {
     return OpPos(t->GetOpArgs(shard), key, elem, rank, count, max_len);
   };
 
@@ -1314,7 +1314,7 @@ void ListFamily::LInsert(CmdArgList args, const CommandContext& cmd_cntx) {
 
   DCHECK(pivot.data() && elem.data());
 
-  auto cb = [&](Transaction* t, EngineShard* shard) {
+  auto cb = [&, &pivot = pivot, &elem = elem](Transaction* t, EngineShard* shard) {
     return OpInsert(t->GetOpArgs(shard), key, pivot, elem, where);
   };
 

src/server/search/search_family_test.cc

@@ -93,9 +93,7 @@ template <typename... Args> auto IsUnordArray(Args... args) {
 template <typename Expected, size_t... Is>
 void BuildKvMatchers(std::vector<Matcher<std::pair<std::string, RespExpr>>>& kv_matchers,
                      const Expected& expected, std::index_sequence<Is...>) {
-  std::initializer_list<int>{
+  (kv_matchers.emplace_back(Pair(std::get<Is * 2>(expected), std::get<Is * 2 + 1>(expected))), ...);
-      (kv_matchers.emplace_back(Pair(std::get<Is * 2>(expected), std::get<Is * 2 + 1>(expected))),
-       0)...};
 }
 
 MATCHER_P(IsMapMatcher, expected, "") {

src/server/stream_family.cc

@@ -1807,7 +1807,7 @@ void DestroyGroup(facade::CmdArgParser* parser, Transaction* tx, SinkReplyBuilde
   if (parser->HasNext())
     return builder->SendError(UnknownSubCmd("DESTROY", "XGROUP"));
 
-  auto cb = [&](Transaction* t, EngineShard* shard) {
+  auto cb = [&, &key = key, &gname = gname](Transaction* t, EngineShard* shard) {
     return OpDestroyGroup(t->GetOpArgs(shard), key, gname);
   };
 
@@ -1833,7 +1833,8 @@ void CreateConsumer(facade::CmdArgParser* parser, Transaction* tx, SinkReplyBuil
   if (parser->HasNext())
     return builder->SendError(UnknownSubCmd("CREATECONSUMER", "XGROUP"));
 
-  auto cb = [&](Transaction* t, EngineShard* shard) {
+  auto cb = [&, &key = key, &gname = gname, &consumer = consumer](Transaction* t,
+                                                                  EngineShard* shard) {
     return OpCreateConsumer(t->GetOpArgs(shard), key, gname, consumer);
   };
   OpResult<uint32_t> result = tx->ScheduleSingleHopT(cb);
@@ -1861,7 +1862,8 @@ void DelConsumer(facade::CmdArgParser* parser, Transaction* tx, SinkReplyBuilder
   if (parser->HasNext())
     return builder->SendError(UnknownSubCmd("DELCONSUMER", "XGROUP"));
 
-  auto cb = [&](Transaction* t, EngineShard* shard) {
+  auto cb = [&, &key = key, &gname = gname, &consumer = consumer](Transaction* t,
+                                                                  EngineShard* shard) {
     return OpDelConsumer(t->GetOpArgs(shard), key, gname, consumer);
   };
 
@@ -1894,7 +1896,7 @@ void SetId(facade::CmdArgParser* parser, Transaction* tx, SinkReplyBuilder* buil
   if (auto err = parser->Error(); err)
     return builder->SendError(err->MakeReply());
 
-  auto cb = [&](Transaction* t, EngineShard* shard) {
+  auto cb = [&, &key = key, &gname = gname, &id = id](Transaction* t, EngineShard* shard) {
     return OpSetId(t->GetOpArgs(shard), key, gname, id);
   };
 

src/server/string_family.cc

@@ -1453,7 +1453,7 @@ void StringFamily::GetRange(CmdArgList args, const CommandContext& cmnd_cntx) {
     return cmnd_cntx.rb->SendError(err->MakeReply());
   }
 
-  auto cb = [&](Transaction* t, EngineShard* shard) {
+  auto cb = [&, &key = key, &start = start, &end = end](Transaction* t, EngineShard* shard) {
     return OpGetRange(t->GetOpArgs(shard), key, start, end);
   };
 
@@ -1477,7 +1477,7 @@ void StringFamily::SetRange(CmdArgList args, const CommandContext& cmnd_cntx) {
     return builder->SendError("string exceeds maximum allowed size");
   }
 
-  auto cb = [&](Transaction* t, EngineShard* shard) {
+  auto cb = [&, &key = key, &start = start, &value = value](Transaction* t, EngineShard* shard) {
     return OpSetRange(t->GetOpArgs(shard), key, start, value);
   };
   auto res = cmnd_cntx.tx->ScheduleSingleHopT(cb);