From 58af00409400607f85f30ceaeda4a6c1ffbfe038 Mon Sep 17 00:00:00 2001 From: Ted Lyngmo Date: Thu, 28 Apr 2022 15:10:22 +0200 Subject: [PATCH] Fix some types of warnings Also added a few warning options to the build: -Wshadow -Wconversion -Wcast-qual -Wsign-conversion can also be added if the below pull request is approved: https://github.com/redis/hiredis/pull/1064 Signed-off-by: Ted Lyngmo --- CMakeLists.txt | 6 ++-- src/sw/redis++/connection.cpp | 56 ++++++++++++++++---------------- src/sw/redis++/crc16.cpp | 5 ++- src/sw/redis++/queued_redis.hpp | 12 +++---- src/sw/redis++/redis.hpp | 12 +++---- src/sw/redis++/redis_cluster.hpp | 16 ++++----- src/sw/redis++/reply.h | 2 +- src/sw/redis++/shards_pool.cpp | 8 ++--- 8 files changed, 59 insertions(+), 58 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index a30558a2..64ab610e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -167,7 +167,8 @@ if(REDIS_PLUS_PLUS_BUILD_STATIC) set_target_properties(${STATIC_LIB} PROPERTIES CXX_STANDARD ${REDIS_PLUS_PLUS_CXX_STANDARD}) set_target_properties(${STATIC_LIB} PROPERTIES OUTPUT_NAME redis++_static) else() - target_compile_options(${STATIC_LIB} PRIVATE "-Wall" "-W" "-Werror") + # Add -Wsign-conversion when hiredis approves https://github.com/redis/hiredis/pull/1064 + target_compile_options(${STATIC_LIB} PRIVATE "-Wall" "-Wextra" "-Wshadow" "-Wconversion" "-Wcast-qual" "-Werror") set_target_properties(${STATIC_LIB} PROPERTIES OUTPUT_NAME redis++) endif() @@ -222,7 +223,8 @@ if(REDIS_PLUS_PLUS_BUILD_SHARED) set_target_properties(${SHARED_LIB} PROPERTIES CXX_STANDARD ${REDIS_PLUS_PLUS_CXX_STANDARD}) set_target_properties(${SHARED_LIB} PROPERTIES WINDOWS_EXPORT_ALL_SYMBOLS TRUE) else() - target_compile_options(${SHARED_LIB} PRIVATE "-Wall" "-W" "-Werror") + # Add -Wsign-conversion when hiredis approves https://github.com/redis/hiredis/pull/1064 + target_compile_options(${SHARED_LIB} PRIVATE "-Wall" "-Wextra" "-Wshadow" "-Wconversion" "-Wcast-qual" "-Werror") endif() set_target_properties(${SHARED_LIB} PROPERTIES OUTPUT_NAME redis++) diff --git a/src/sw/redis++/connection.cpp b/src/sw/redis++/connection.cpp index f0a80882..4f993ba0 100644 --- a/src/sw/redis++/connection.cpp +++ b/src/sw/redis++/connection.cpp @@ -36,29 +36,29 @@ ConnectionOptions::ConnectionOptions(const std::string &uri) : ConnectionOptions(_parse_uri(uri)) {} ConnectionOptions ConnectionOptions::_parse_uri(const std::string &uri) const { - std::string type; + std::string scheme; std::string auth; - std::string path; - std::tie(type, auth, path) = _split_uri(uri); + std::string spath; + std::tie(scheme, auth, spath) = _split_uri(uri); ConnectionOptions opts; _set_auth_opts(auth, opts); - auto db = 0; + auto ldb = 0; std::string parameter_string; - std::tie(path, db, parameter_string) = _split_path(path); + std::tie(spath, ldb, parameter_string) = _split_path(spath); _parse_parameters(parameter_string, opts); - opts.db = db; + opts.db = ldb; - if (type == "tcp") { - _set_tcp_opts(path, opts); - } else if (type == "unix") { - _set_unix_opts(path, opts); + if (scheme == "tcp") { + _set_tcp_opts(spath, opts); + } else if (scheme == "unix") { + _set_unix_opts(spath, opts); } else { - throw Error("invalid URI: invalid type"); + throw Error("invalid URI: invalid scheme"); } return opts; @@ -167,42 +167,42 @@ auto ConnectionOptions::_split_uri(const std::string &uri) const throw Error("invalid URI: no scheme"); } - auto type = uri.substr(0, pos); + auto scheme = uri.substr(0, pos); auto start = pos + 3; pos = uri.find("@", start); if (pos == std::string::npos) { // No auth info. - return std::make_tuple(type, std::string{}, uri.substr(start)); + return std::make_tuple(scheme, std::string{}, uri.substr(start)); } auto auth = uri.substr(start, pos - start); - return std::make_tuple(type, auth, uri.substr(pos + 1)); + return std::make_tuple(scheme, auth, uri.substr(pos + 1)); } -auto ConnectionOptions::_split_path(const std::string &path) const +auto ConnectionOptions::_split_path(const std::string &ipath) const -> std::tuple { - auto parameter_pos = path.rfind("?"); + auto parameter_pos = ipath.rfind("?"); std::string parameter_string; if (parameter_pos != std::string::npos) { - parameter_string = path.substr(parameter_pos + 1); + parameter_string = ipath.substr(parameter_pos + 1); } - auto pos = path.rfind("/"); + auto pos = ipath.rfind("/"); if (pos != std::string::npos) { // Might specified a db number. try { - auto db = std::stoi(path.substr(pos + 1)); + auto ldb = std::stoi(ipath.substr(pos + 1)); - return std::make_tuple(path.substr(0, pos), db, parameter_string); + return std::make_tuple(ipath.substr(0, pos), ldb, parameter_string); } catch (const std::exception &) { // Not a db number, and it might be a path to unix domain socket. } } // No db number specified, and use default one, i.e. 0. - return std::make_tuple(path.substr(0, parameter_pos), 0, parameter_string); + return std::make_tuple(ipath.substr(0, parameter_pos), 0, parameter_string); } void ConnectionOptions::_set_auth_opts(const std::string &auth, ConnectionOptions &opts) const { @@ -221,25 +221,25 @@ void ConnectionOptions::_set_auth_opts(const std::string &auth, ConnectionOption } } -void ConnectionOptions::_set_tcp_opts(const std::string &path, ConnectionOptions &opts) const { +void ConnectionOptions::_set_tcp_opts(const std::string &ipath, ConnectionOptions &opts) const { opts.type = ConnectionType::TCP; - auto pos = path.find(":"); + auto pos = ipath.find(":"); if (pos != std::string::npos) { // Port number specified. try { - opts.port = std::stoi(path.substr(pos + 1)); + opts.port = std::stoi(ipath.substr(pos + 1)); } catch (const std::exception &) { throw Error("invalid URI: invalid port"); } } // else use default port, i.e. 6379. - opts.host = path.substr(0, pos); + opts.host = ipath.substr(0, pos); } -void ConnectionOptions::_set_unix_opts(const std::string &path, ConnectionOptions &opts) const { +void ConnectionOptions::_set_unix_opts(const std::string &ipath, ConnectionOptions &opts) const { opts.type = ConnectionType::UNIX; - opts.path = path; + opts.path = ipath; } class Connection::Connector { @@ -404,7 +404,7 @@ void Connection::send(CmdArgs &args) { assert(ctx != nullptr); if (redisAppendCommandArgv(ctx, - args.size(), + static_cast(args.size()), args.argv(), args.argv_len()) != REDIS_OK) { throw_error(*ctx, "Failed to send command"); diff --git a/src/sw/redis++/crc16.cpp b/src/sw/redis++/crc16.cpp index c94a08d3..6d85a191 100644 --- a/src/sw/redis++/crc16.cpp +++ b/src/sw/redis++/crc16.cpp @@ -84,10 +84,9 @@ static const uint16_t crc16tab[256]= { }; uint16_t crc16(const char *buf, int len) { - int counter; uint16_t crc = 0; - for (counter = 0; counter < len; counter++) - crc = (crc<<8) ^ crc16tab[((crc>>8) ^ *buf++)&0x00FF]; + for (int counter = 0; counter < len; counter++) + crc = static_cast((crc<<8) ^ crc16tab[((crc>>8) ^ *buf++)&0x00FF]); return crc; } diff --git a/src/sw/redis++/queued_redis.hpp b/src/sw/redis++/queued_redis.hpp index d2865e46..ba8c6871 100644 --- a/src/sw/redis++/queued_redis.hpp +++ b/src/sw/redis++/queued_redis.hpp @@ -78,9 +78,9 @@ auto QueuedRedis::command(Cmd cmd, Args &&...args) template template QueuedRedis& QueuedRedis::command(const StringView &cmd_name, Args &&...args) { - auto cmd = [](Connection &connection, const StringView &cmd_name, Args &&...args) { + auto cmd = [](Connection &connection, const StringView &lcmd_name, Args &&...largs) { CmdArgs cmd_args; - cmd_args.append(cmd_name, std::forward(args)...); + cmd_args.append(lcmd_name, std::forward(largs)...); connection.send(cmd_args); }; @@ -95,11 +95,11 @@ auto QueuedRedis::command(Input first, Input last) throw Error("command: empty range"); } - auto cmd = [](Connection &connection, Input first, Input last) { + auto cmd = [](Connection &connection, Input lfirst, Input llast) { CmdArgs cmd_args; - while (first != last) { - cmd_args.append(*first); - ++first; + while (lfirst != llast) { + cmd_args.append(*lfirst); + ++lfirst; } connection.send(cmd_args); }; diff --git a/src/sw/redis++/redis.hpp b/src/sw/redis++/redis.hpp index c560ce11..32f7db32 100644 --- a/src/sw/redis++/redis.hpp +++ b/src/sw/redis++/redis.hpp @@ -51,9 +51,9 @@ auto Redis::command(Cmd cmd, Args &&...args) template auto Redis::command(const StringView &cmd_name, Args &&...args) -> typename std::enable_if::type>::value, ReplyUPtr>::type { - auto cmd = [](Connection &connection, const StringView &cmd_name, Args &&...args) { + auto cmd = [](Connection &connection, const StringView &lcmd_name, Args &&...largs) { CmdArgs cmd_args; - cmd_args.append(cmd_name, std::forward(args)...); + cmd_args.append(lcmd_name, std::forward(largs)...); connection.send(cmd_args); }; @@ -65,11 +65,11 @@ auto Redis::command(Input first, Input last) -> typename std::enable_if::value, ReplyUPtr>::type { range_check("command", first, last); - auto cmd = [](Connection &connection, Input first, Input last) { + auto cmd = [](Connection &connection, Input lfirst, Input llast) { CmdArgs cmd_args; - while (first != last) { - cmd_args.append(*first); - ++first; + while (lfirst != llast) { + cmd_args.append(*lfirst); + ++lfirst; } connection.send(cmd_args); }; diff --git a/src/sw/redis++/redis_cluster.hpp b/src/sw/redis++/redis_cluster.hpp index 2153d181..67901047 100644 --- a/src/sw/redis++/redis_cluster.hpp +++ b/src/sw/redis++/redis_cluster.hpp @@ -83,12 +83,12 @@ auto RedisCluster::command(Input first, Input last) const auto &key = *first; ++first; - auto cmd = [&key](Connection &connection, Input first, Input last) { + auto cmd = [&key](Connection &connection, Input lfirst, Input llast) { CmdArgs cmd_args; cmd_args.append(key); - while (first != last) { - cmd_args.append(*first); - ++first; + while (lfirst != llast) { + cmd_args.append(*lfirst); + ++lfirst; } connection.send(cmd_args); }; @@ -1337,12 +1337,12 @@ ReplyUPtr RedisCluster::_command(Cmd cmd, const StringView &key, Args &&...args) SafeConnection safe_connection(*pool); return _command(cmd, safe_connection.connection(), std::forward(args)...); - } catch (const IoError &err) { + } catch (const IoError &) { // When master is down, one of its replicas will be promoted to be the new master. // If we try to send command to the old master, we'll get an *IoError*. // In this case, we need to update the slots mapping. _pool.update(); - } catch (const ClosedError &err) { + } catch (const ClosedError &) { // Node might be removed. // 1. Get up-to-date slot mapping to check if the node still exists. _pool.update(); @@ -1350,7 +1350,7 @@ ReplyUPtr RedisCluster::_command(Cmd cmd, const StringView &key, Args &&...args) // TODO: // 2. If it's NOT exist, update slot mapping, and retry. // 3. If it's still exist, that means the node is down, NOT removed, throw exception. - } catch (const MovedError &err) { + } catch (const MovedError &) { // Slot mapping has been changed, update it and try again. _pool.update(); } catch (const AskError &err) { @@ -1365,7 +1365,7 @@ ReplyUPtr RedisCluster::_command(Cmd cmd, const StringView &key, Args &&...args) // 2. resend last command. try { return _command(cmd, connection, std::forward(args)...); - } catch (const MovedError &err) { + } catch (const MovedError &) { throw Error("Slot migrating... ASKING node hasn't been set to IMPORTING state"); } } // For other exceptions, just throw it. diff --git a/src/sw/redis++/reply.h b/src/sw/redis++/reply.h index d8917432..eb108a61 100644 --- a/src/sw/redis++/reply.h +++ b/src/sw/redis++/reply.h @@ -47,7 +47,7 @@ template struct ParseTag {}; template -inline T parse(redisReply &reply) { +T parse(redisReply &reply) { return parse(ParseTag(), reply); } diff --git a/src/sw/redis++/shards_pool.cpp b/src/sw/redis++/shards_pool.cpp index e1223163..2951fd2d 100644 --- a/src/sw/redis++/shards_pool.cpp +++ b/src/sw/redis++/shards_pool.cpp @@ -234,7 +234,7 @@ Node ShardsPool::_parse_node(redisReply *reply) const { } auto host = reply::parse(*(reply->element[0])); - int port = reply::parse(*(reply->element[1])); + auto port = static_cast(reply::parse(*(reply->element[1]))); return {host, port}; } @@ -282,11 +282,11 @@ Slot ShardsPool::_slot(const StringView &key) const { // And I did some minor changes. const auto *k = key.data(); - auto keylen = key.size(); + auto keylen = static_cast(key.size()); // start-end indexes of { and }. - std::size_t s = 0; - std::size_t e = 0; + int s = 0; + int e = 0; // Search the first occurrence of '{'. for (s = 0; s < keylen; s++)