From 45aba139b68ec6395ac831f6c4a9fb6e6e28b090 Mon Sep 17 00:00:00 2001 From: Roman Gershman Date: Sun, 6 Oct 2024 21:45:11 +0300 Subject: [PATCH] chore: reduce usage of ToUpper (#3874) We would like to stop passing MutableSlice as arguments and removing ToUpper is the first step to it. Signed-off-by: Roman Gershman --- src/server/acl/acl_family.cc | 68 ++++++++++++++-------------- src/server/cluster/cluster_family.cc | 11 ++--- src/server/memory_cmd.cc | 6 +-- src/server/stream_family.cc | 34 ++++++-------- src/server/zset_family.cc | 25 ++++------ 5 files changed, 62 insertions(+), 82 deletions(-) diff --git a/src/server/acl/acl_family.cc b/src/server/acl/acl_family.cc index 1cb569f7d251..4300b31eb618 100644 --- a/src/server/acl/acl_family.cc +++ b/src/server/acl/acl_family.cc @@ -41,22 +41,22 @@ #include "server/server_state.h" #include "util/proactor_pool.h" -ABSL_FLAG(std::string, aclfile, "", "Path and name to aclfile"); +using namespace std; + +ABSL_FLAG(string, aclfile, "", "Path and name to aclfile"); namespace dfly::acl { namespace { -std::string PasswordsToString(const absl::flat_hash_set& passwords, bool nopass, - bool full_sha); -using MaterializedContents = std::optional>>; +string PasswordsToString(const absl::flat_hash_set& passwords, bool nopass, bool full_sha); +using MaterializedContents = optional>>; -MaterializedContents MaterializeFileContents(std::vector* usernames, - std::string_view file_contents); +MaterializedContents MaterializeFileContents(vector* usernames, string_view file_contents); -std::string AclKeysToString(const AclKeys& keys); +string AclKeysToString(const AclKeys& keys); -std::string AclPubSubToString(const AclPubSub& pub_sub); +string AclPubSubToString(const AclPubSub& pub_sub); } // namespace AclFamily::AclFamily(UserRegistry* registry, util::ProactorPool* pool) @@ -74,19 +74,19 @@ void AclFamily::List(CmdArgList args, ConnectionContext* cntx) { rb->StartArray(registry.size()); for (const auto& [username, user] : registry) { - std::string buffer = "user "; - const std::string password = PasswordsToString(user.Passwords(), user.HasNopass(), false); + string buffer = "user "; + const string password = PasswordsToString(user.Passwords(), user.HasNopass(), false); - const std::string acl_keys = AclKeysToString(user.Keys()); + const string acl_keys = AclKeysToString(user.Keys()); - const std::string acl_pub_sub = AclPubSubToString(user.PubSub()); + const string acl_pub_sub = AclPubSubToString(user.PubSub()); - const std::string maybe_space_com = acl_keys.empty() ? "" : " "; + const string maybe_space_com = acl_keys.empty() ? "" : " "; - const std::string acl_cat_and_commands = + const string acl_cat_and_commands = AclCatAndCommandToString(user.CatChanges(), user.CmdChanges()); - using namespace std::string_view_literals; + using namespace string_view_literals; absl::StrAppend(&buffer, username, " ", user.IsActive() ? "on "sv : "off "sv, password, acl_keys, maybe_space_com, acl_pub_sub, " ", acl_cat_and_commands); @@ -117,7 +117,7 @@ void AclFamily::StreamUpdatesToAllProactorConnections(const std::string& user, using facade::ErrorReply; void AclFamily::SetUser(CmdArgList args, ConnectionContext* cntx) { - std::string_view username = facade::ToSV(args[0]); + string_view username = facade::ToSV(args[0]); auto reg = registry_->GetRegistryWithWriteLock(); const bool exists = reg.registry.contains(username); const bool has_all_keys = exists ? reg.registry.find(username)->second.Keys().all_keys : false; @@ -140,8 +140,8 @@ void AclFamily::SetUser(CmdArgList args, ConnectionContext* cntx) { cntx->SendOk(); if (exists) { if (!reset_channels) { - StreamUpdatesToAllProactorConnections(std::string(username), user.AclCommands(), - user.Keys(), user.PubSub()); + StreamUpdatesToAllProactorConnections(string(username), user.AclCommands(), user.Keys(), + user.PubSub()); } // We evict connections that had their channels reseted else { @@ -153,8 +153,7 @@ void AclFamily::SetUser(CmdArgList args, ConnectionContext* cntx) { std::visit(Overloaded{error_case, update_case}, std::move(req)); } -void AclFamily::EvictOpenConnectionsOnAllProactors( - const absl::flat_hash_set& users) { +void AclFamily::EvictOpenConnectionsOnAllProactors(const absl::flat_hash_set& users) { auto close_cb = [&users]([[maybe_unused]] size_t id, util::Connection* conn) { CHECK(conn); auto connection = static_cast(conn); @@ -187,10 +186,10 @@ void AclFamily::EvictOpenConnectionsOnAllProactorsWithRegistry( void AclFamily::DelUser(CmdArgList args, ConnectionContext* cntx) { auto& registry = *registry_; - absl::flat_hash_set users; + absl::flat_hash_set users; for (auto arg : args) { - std::string_view username = facade::ToSV(arg); + string_view username = facade::ToSV(arg); if (username == "default") { continue; } @@ -214,24 +213,24 @@ void AclFamily::WhoAmI(CmdArgList args, ConnectionContext* cntx) { rb->SendBulkString(absl::StrCat("User is ", cntx->authed_username)); } -std::string AclFamily::RegistryToString() const { +string AclFamily::RegistryToString() const { auto registry_with_read_lock = registry_->GetRegistryWithLock(); auto& registry = registry_with_read_lock.registry; - std::string result; + string result; for (auto& [username, user] : registry) { - std::string command = "USER "; - const std::string password = PasswordsToString(user.Passwords(), user.HasNopass(), true); + string command = "USER "; + const string password = PasswordsToString(user.Passwords(), user.HasNopass(), true); - const std::string acl_keys = AclKeysToString(user.Keys()); + const string acl_keys = AclKeysToString(user.Keys()); - const std::string maybe_space = acl_keys.empty() ? "" : " "; + const string maybe_space = acl_keys.empty() ? "" : " "; - const std::string acl_pub_sub = AclPubSubToString(user.PubSub()); + const string acl_pub_sub = AclPubSubToString(user.PubSub()); - const std::string acl_cat_and_commands = + const string acl_cat_and_commands = AclCatAndCommandToString(user.CatChanges(), user.CmdChanges()); - using namespace std::string_view_literals; + using namespace string_view_literals; absl::StrAppend(&result, command, username, " ", user.IsActive() ? "ON "sv : "OFF "sv, password, acl_keys, maybe_space, acl_pub_sub, " ", acl_cat_and_commands, "\n"); @@ -471,8 +470,8 @@ void AclFamily::Cat(CmdArgList args, ConnectionContext* cntx) { } if (args.size() == 1) { - ToUpper(&args[0]); - std::string_view category = facade::ToSV(args[0]); + string category = absl::AsciiStrToUpper(ArgS(args, 0)); + if (!cat_table_.contains(category)) { auto error = absl::StrCat("Unkown category: ", category); cntx->SendError(error); @@ -605,8 +604,7 @@ void AclFamily::DryRun(CmdArgList args, ConnectionContext* cntx) { return; } - ToUpper(&args[1]); - auto command = facade::ArgS(args, 1); + string command = absl::AsciiStrToUpper(ArgS(args, 1)); auto* cid = cmd_registry_->Find(command); if (!cid) { auto error = absl::StrCat("Command '", command, "' not found"); diff --git a/src/server/cluster/cluster_family.cc b/src/server/cluster/cluster_family.cc index 2ff729c90aca..230d87a4939c 100644 --- a/src/server/cluster/cluster_family.cc +++ b/src/server/cluster/cluster_family.cc @@ -387,9 +387,7 @@ void ClusterFamily::KeySlot(CmdArgList args, ConnectionContext* cntx) { void ClusterFamily::Cluster(CmdArgList args, ConnectionContext* cntx) { // In emulated cluster mode, all slots are mapped to the same host, and number of cluster // instances is thus 1. - - ToUpper(&args[0]); - string_view sub_cmd = ArgS(args, 0); + string sub_cmd = absl::AsciiStrToUpper(ArgS(args, 0)); if (!IsClusterEnabledOrEmulated()) { return cntx->SendError(kClusterDisabled); @@ -445,8 +443,7 @@ void ClusterFamily::DflyCluster(CmdArgList args, ConnectionContext* cntx) { VLOG(2) << "Got DFLYCLUSTER command (NO_CLIENT_ID): " << args; } - ToUpper(&args[0]); - string_view sub_cmd = ArgS(args, 0); + string sub_cmd = absl::AsciiStrToUpper(ArgS(args, 0)); args.remove_prefix(1); // remove subcommand name if (sub_cmd == "GETSLOTINFO") { return DflyClusterGetSlotInfo(args, cntx); @@ -747,8 +744,8 @@ void ClusterFamily::DflySlotMigrationStatus(CmdArgList args, ConnectionContext* } void ClusterFamily::DflyMigrate(CmdArgList args, ConnectionContext* cntx) { - ToUpper(&args[0]); - string_view sub_cmd = ArgS(args, 0); + string sub_cmd = absl::AsciiStrToUpper(ArgS(args, 0)); + args.remove_prefix(1); if (sub_cmd == "INIT") { InitMigration(args, cntx); diff --git a/src/server/memory_cmd.cc b/src/server/memory_cmd.cc index 7e590267b2c0..e81955494d93 100644 --- a/src/server/memory_cmd.cc +++ b/src/server/memory_cmd.cc @@ -303,16 +303,16 @@ void MemoryCmd::ArenaStats(CmdArgList args) { bool backing = false; bool show_arenas = false; if (args.size() >= 2) { - ToUpper(&args[1]); + string sub_cmd = absl::AsciiStrToUpper(ArgS(args, 1)); - if (ArgS(args, 1) == "SHOW") { + if (sub_cmd == "SHOW") { if (args.size() != 2) return cntx_->SendError(kSyntaxErr, kSyntaxErrType); show_arenas = true; } else { unsigned tid_indx = 1; - if (ArgS(args, tid_indx) == "BACKING") { + if (sub_cmd == "BACKING") { ++tid_indx; backing = true; } diff --git a/src/server/stream_family.cc b/src/server/stream_family.cc index a0503f8b1c32..12911e895195 100644 --- a/src/server/stream_family.cc +++ b/src/server/stream_family.cc @@ -1888,9 +1888,7 @@ optional> ParseAddOrTrimArgsOrReply(CmdArgList args, unsigned id_indx = 1; for (; id_indx < args.size(); ++id_indx) { - ToUpper(&args[id_indx]); - string_view arg = ArgS(args, id_indx); - + string arg = absl::AsciiStrToUpper(ArgS(args, id_indx)); size_t remaining_args = args.size() - id_indx - 1; if (is_xadd && arg == "NOMKSTREAM") { @@ -2059,8 +2057,7 @@ absl::InlinedVector GetXclaimIds(CmdArgList& args) { void ParseXclaimOptions(CmdArgList& args, ClaimOpts& opts, ConnectionContext* cntx) { for (size_t i = 0; i < args.size(); ++i) { - ToUpper(&args[i]); - string_view arg = ArgS(args, i); + string arg = absl::AsciiStrToUpper(ArgS(args, i)); bool remaining_args = args.size() - i - 1 > 0; if (remaining_args) { @@ -2186,8 +2183,8 @@ void StreamFamily::XGroup(CmdArgList args, ConnectionContext* cntx) { void StreamFamily::XInfo(CmdArgList args, ConnectionContext* cntx) { auto* rb = static_cast(cntx->reply_builder()); - ToUpper(&args[0]); - string_view sub_cmd = ArgS(args, 0); + string sub_cmd = absl::AsciiStrToUpper(ArgS(args, 0)); + if (sub_cmd == "HELP") { string_view help_arr[] = {"CONSUMERS ", " Show consumers of .", @@ -2254,15 +2251,13 @@ void StreamFamily::XInfo(CmdArgList args, ConnectionContext* cntx) { if (args.size() >= 3) { full = 1; - ToUpper(&args[2]); - string_view full_arg = ArgS(args, 2); + string full_arg = absl::AsciiStrToUpper(ArgS(args, 2)); if (full_arg != "FULL") { return cntx->SendError( "unknown subcommand or wrong number of arguments for 'STREAM'. Try XINFO HELP."); } if (args.size() > 3) { - ToUpper(&args[3]); - string_view count_arg = ArgS(args, 3); + string count_arg = absl::AsciiStrToUpper(ArgS(args, 3)); string_view count_value_arg = ArgS(args, 4); if (count_arg != "COUNT") { return cntx->SendError( @@ -2447,8 +2442,7 @@ void StreamFamily::XLen(CmdArgList args, ConnectionContext* cntx) { bool ParseXpendingOptions(CmdArgList& args, PendingOpts& opts, ConnectionContext* cntx) { size_t id_indx = 0; - ToUpper(&args[id_indx]); - string_view arg = ArgS(args, id_indx); + string arg = absl::AsciiStrToUpper(ArgS(args, id_indx)); if (arg == "IDLE" && args.size() > 4) { id_indx++; @@ -2579,8 +2573,7 @@ std::optional ParseReadArgsOrReply(CmdArgList args, bool read_group, size_t id_indx = 0; if (opts.read_group) { - ToUpper(&args[id_indx]); - string_view arg = ArgS(args, id_indx); + string arg = absl::AsciiStrToUpper(ArgS(args, id_indx)); if (arg.size() - 1 < 2) { cntx->SendError(kSyntaxErr); @@ -2599,8 +2592,7 @@ std::optional ParseReadArgsOrReply(CmdArgList args, bool read_group, } for (; id_indx < args.size(); ++id_indx) { - ToUpper(&args[id_indx]); - string_view arg = ArgS(args, id_indx); + string arg = absl::AsciiStrToUpper(ArgS(args, id_indx)); bool remaining_args = args.size() - id_indx - 1 > 0; if (arg == "BLOCK" && remaining_args) { @@ -3078,8 +3070,8 @@ void StreamFamily::XRangeGeneric(CmdArgList args, bool is_rev, ConnectionContext if (args.size() != 5) { return cntx->SendError(WrongNumArgsError("XRANGE"), kSyntaxErrType); } - ToUpper(&args[3]); - string_view opt = ArgS(args, 3); + + string opt = absl::AsciiStrToUpper(ArgS(args, 3)); string_view val = ArgS(args, 4); if (opt != "COUNT" || !absl::SimpleAtoi(val, &range_opts.count)) { @@ -3159,8 +3151,8 @@ void StreamFamily::XAutoClaim(CmdArgList args, ConnectionContext* cntx) { opts.start = rs.parsed_id.val; for (size_t i = 5; i < args.size(); ++i) { - ToUpper(&args[i]); - string_view arg = ArgS(args, i); + string arg = absl::AsciiStrToUpper(ArgS(args, i)); + bool remaining_args = args.size() - i - 1 > 0; if (remaining_args) { diff --git a/src/server/zset_family.cc b/src/server/zset_family.cc index bdf4a8885a38..573fdd7259cf 100644 --- a/src/server/zset_family.cc +++ b/src/server/zset_family.cc @@ -1106,8 +1106,8 @@ OpResult ParseAggregate(CmdArgList args, bool store, SetOpArgs* op_arg return OpStatus::SYNTAX_ERR; } - ToUpper(&args[1]); - auto filled = FillAggType(ArgS(args, 1), op_args); + string agg_type = absl::AsciiStrToUpper(ArgS(args, 1)); + auto filled = FillAggType(agg_type, op_args); if (!filled) { return filled.status(); } @@ -1156,8 +1156,7 @@ OpResult ParseSetOpArgs(CmdArgList args, bool store) { DCHECK_LE(opt_args_start, args.size()); // Checked inside DetermineKeys for (size_t i = opt_args_start; i < args.size(); ++i) { - ToUpper(&args[i]); - string_view arg = ArgS(args, i); + string arg = absl::AsciiStrToUpper(ArgS(args, i)); if (arg == "WEIGHTS") { auto parsed_cnt = ParseWeights(args.subspan(i), &op_args); if (!parsed_cnt) { @@ -1815,9 +1814,7 @@ void ZSetFamily::ZAdd(CmdArgList args, ConnectionContext* cntx) { ZParams zparams; size_t i = 1; for (; i < args.size() - 1; ++i) { - ToUpper(&args[i]); - - string_view cur_arg = ArgS(args, i); + string cur_arg = absl::AsciiStrToUpper(ArgS(args, i)); if (cur_arg == "XX") { zparams.flags |= ZADD_IN_XX; // update only @@ -2560,9 +2557,7 @@ void ZSetFamily::GeoAdd(CmdArgList args, ConnectionContext* cntx) { ZParams zparams; size_t i = 1; for (; i < args.size(); ++i) { - ToUpper(&args[i]); - - string_view cur_arg = ArgS(args, i); + string cur_arg = absl::AsciiStrToUpper(ArgS(args, i)); if (cur_arg == "XX") { zparams.flags |= ZADD_IN_XX; // update only @@ -2661,8 +2656,8 @@ void ZSetFamily::GeoPos(CmdArgList args, ConnectionContext* cntx) { void ZSetFamily::GeoDist(CmdArgList args, ConnectionContext* cntx) { double distance_multiplier = 1; if (args.size() == 4) { - ToUpper(&args[3]); - string_view unit = ArgS(args, 3); + string unit = absl::AsciiStrToUpper(ArgS(args, 3)); + distance_multiplier = ExtractUnit(unit); args.remove_suffix(1); if (distance_multiplier < 0) { @@ -2933,9 +2928,8 @@ void ZSetFamily::GeoSearch(CmdArgList args, ConnectionContext* cntx) { bool by_set = false; for (size_t i = 1; i < args.size(); ++i) { - ToUpper(&args[i]); + string cur_arg = absl::AsciiStrToUpper(ArgS(args, i)); - string_view cur_arg = ArgS(args, i); if (cur_arg == "FROMMEMBER") { if (from_set) { return cntx->SendError(kFromMemberLonglatErr); @@ -3071,9 +3065,8 @@ void ZSetFamily::GeoRadiusByMember(CmdArgList args, ConnectionContext* cntx) { shape.type = CIRCULAR_TYPE; for (size_t i = 4; i < args.size(); ++i) { - ToUpper(&args[i]); + string cur_arg = absl::AsciiStrToUpper(ArgS(args, i)); - string_view cur_arg = ArgS(args, i); if (cur_arg == "ASC") { if (geo_ops.sorting != Sorting::kUnsorted) { return cntx->SendError(kAscDescErr);