From 6f3c6e3d57a252f57bfd52234b188c00a88eec59 Mon Sep 17 00:00:00 2001 From: Shahar Mike Date: Mon, 20 Jan 2025 10:24:07 +0200 Subject: [PATCH] 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 | 1 - helio | 2 +- src/core/dash_internal.h | 7 ++++--- src/core/json/jsonpath_grammar.y | 4 ---- src/core/json/jsonpath_test.cc | 4 ++-- src/core/search/parser.y | 8 ++------ src/core/search/vector_utils.cc | 12 ++++++++---- src/core/task_queue.cc | 2 +- src/core/task_queue.h | 1 - src/server/acl/validator.cc | 8 -------- src/server/bitops_family.cc | 4 ++-- src/server/detail/snapshot_storage.cc | 6 ++++-- src/server/dfly_bench.cc | 6 +++--- src/server/dflycmd.cc | 16 ++++++++-------- src/server/dflycmd.h | 8 -------- src/server/dragonfly_test.cc | 2 +- src/server/json_family.cc | 7 ++++--- src/server/list_family.cc | 4 ++-- src/server/search/search_family_test.cc | 4 +--- src/server/stream_family.cc | 10 ++++++---- src/server/string_family.cc | 4 ++-- 21 files changed, 51 insertions(+), 69 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8a4a441ee..4fb4f495e 100644 --- a/.github/workflows/ci.yml +++ b/.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: diff --git a/helio b/helio index 90bc3cc6a..05c316e17 160000 --- a/helio +++ b/helio @@ -1 +1 @@ -Subproject commit 90bc3cc6aabbffc8b274dc0f7801695d68658529 +Subproject commit 05c316e171e31a6f23165b426b7caf0174f90838 diff --git a/src/core/dash_internal.h b/src/core/dash_internal.h index 418ec4963..7081ca33b 100644 --- a/src/core/dash_internal.h +++ b/src/core/dash_internal.h @@ -20,10 +20,10 @@ namespace detail { template 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 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. diff --git a/src/core/json/jsonpath_grammar.y b/src/core/json/jsonpath_grammar.y index 82a302b98..2a0db50ff 100644 --- a/src/core/json/jsonpath_grammar.y +++ b/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; diff --git a/src/core/json/jsonpath_test.cc b/src/core/json/jsonpath_test.cc index 61e7fa568..268e161d2 100644 --- a/src/core/json/jsonpath_test.cc +++ b/src/core/json/jsonpath_test.cc @@ -465,7 +465,7 @@ TYPED_TEST(JsonPathTest, Mutate) { Path path = this->driver_.TakePath(); TypeParam json = ValidJson(R"([1, 2, 3, 5, 6])"); - MutateCallback cb = [&](optional, JsonType* val) { + auto cb = [](optional, JsonType* val) { int intval = val->as(); *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 key, JsonType* val) { + auto cb2 = [](optional key, JsonType* val) { if (val->is_int64() && !key) { // array element *val = 42; return false; diff --git a/src/core/search/parser.y b/src/core/search/parser.y index d01a8a796..dcc62a0a0 100644 --- a/src/core/search/parser.y +++ b/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; } diff --git a/src/core/search/vector_utils.cc b/src/core/search/vector_utils.cc index 1df311dd8..6cf435ed5 100644 --- a/src/core/search/vector_utils.cc +++ b/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, - size_t dims) { +FAST_MATH float L2Distance(const float* u, const float* v, 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, - size_t dims) { +FAST_MATH float CosineDistance(const float* u, const float* v, 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]; diff --git a/src/core/task_queue.cc b/src/core/task_queue.cc index 39fe7c236..e4d3700ff 100644 --- a/src/core/task_queue.cc +++ b/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); } diff --git a/src/core/task_queue.h b/src/core/task_queue.h index 4004731d4..9a74531c1 100644 --- a/src/core/task_queue.h +++ b/src/core/task_queue.h @@ -66,7 +66,6 @@ class TaskQueue { private: util::fb2::FiberQueue queue_; std::vector consumer_fibers_; - unsigned pool_max_size_; static __thread unsigned blocked_submitters_; }; diff --git a/src/server/acl/validator.cc b/src/server/acl/validator.cc index 20bc1b994..b7e7627b3 100644 --- a/src/server/acl/validator.cc +++ b/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& 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& acl_commands, const Com return {allowed, AclLog::Reason::PUB_SUB}; } -#pragma GCC diagnostic pop - } // namespace dfly::acl diff --git a/src/server/bitops_family.cc b/src/server/bitops_family.cc index 1be55cfb0..509c53b1e 100644 --- a/src/server/bitops_family.cc +++ b/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 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); }; diff --git a/src/server/detail/snapshot_storage.cc b/src/server/detail/snapshot_storage.cc index 93e534f8c..87faeb966 100644 --- a/src/server/detail/snapshot_storage.cc +++ b/src/server/detail/snapshot_storage.cc @@ -334,7 +334,8 @@ io::Result, 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, GenericError> { + auto paths = proactor->Await([&, &bucket_name = + bucket_name]() -> io::Result, GenericError> { vector res; cloud::GCS gcs(&creds_provider_, ctx_, proactor); @@ -458,7 +459,8 @@ io::Result, 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, GenericError> { + auto paths = proactor->Await([&, &bucket_name = + bucket_name]() -> io::Result, GenericError> { const io::Result, GenericError> keys = ListObjects(bucket_name, prefix); if (!keys) { return nonstd::make_unexpected(keys.error()); diff --git a/src/server/dfly_bench.cc b/src/server/dfly_bench.cc index 84518cfe1..1033ba038 100644 --- a/src/server/dfly_bench.cc +++ b/src/server/dfly_bench.cc @@ -234,8 +234,8 @@ class Driver { } Driver(const Driver&) = delete; - Driver(Driver&&) = default; - Driver& operator=(Driver&&) = default; + Driver(Driver&&) = delete; + 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--; diff --git a/src/server/dflycmd.cc b/src/server/dflycmd.cc index 547fefb3e..7415bd03a 100644 --- a/src/server/dflycmd.cc +++ b/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 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()); diff --git a/src/server/dflycmd.h b/src/server/dflycmd.h index 328f2b13c..4a8129e7d 100644 --- a/src/server/dflycmd.h +++ b/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(); diff --git a/src/server/dragonfly_test.cc b/src/server/dragonfly_test.cc index 18230fe62..ebbba6df4 100644 --- a/src/server/dragonfly_test.cc +++ b/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); diff --git a/src/server/json_family.cc b/src/server/json_family.cc index c11c549ae..00d5b492c 100644 --- a/src/server/json_family.cc +++ b/src/server/json_family.cc @@ -955,8 +955,8 @@ OpResult OpDel(const OpArgs& op_args, string_view key, string_view path, if (json_path.HoldsJsonPath()) { const json::Path& path = json_path.AsJsonPath(); - long deletions = - json::MutatePath(path, [](optional, JsonType* val) { return true; }, json_val); + long deletions = json::MutatePath( + path, [](optional, 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); }; diff --git a/src/server/list_family.cc b/src/server/list_family.cc index ab16b0e63..ea800eff1 100644 --- a/src/server/list_family.cc +++ b/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); }; diff --git a/src/server/search/search_family_test.cc b/src/server/search/search_family_test.cc index 05244a3a3..9b845c9d1 100644 --- a/src/server/search/search_family_test.cc +++ b/src/server/search/search_family_test.cc @@ -93,9 +93,7 @@ template auto IsUnordArray(Args... args) { template void BuildKvMatchers(std::vector>>& kv_matchers, const Expected& expected, std::index_sequence) { - std::initializer_list{ - (kv_matchers.emplace_back(Pair(std::get(expected), std::get(expected))), - 0)...}; + (kv_matchers.emplace_back(Pair(std::get(expected), std::get(expected))), ...); } MATCHER_P(IsMapMatcher, expected, "") { diff --git a/src/server/stream_family.cc b/src/server/stream_family.cc index 293093e0e..e284f3f48 100644 --- a/src/server/stream_family.cc +++ b/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 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); }; diff --git a/src/server/string_family.cc b/src/server/string_family.cc index ac4679d5f..706126ad5 100644 --- a/src/server/string_family.cc +++ b/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);