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: redis/hiredis#1064 Signed-off-by: Ted Lyngmo <ted@lyncon.se>
TedLyngmo · Apr 28, 2022 · 56ab298 · 56ab298
1 parent 463f341
commit 56ab298
Show file tree

Hide file tree

Showing 8 changed files with 59 additions and 58 deletions.
diff --git 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" "-W" "-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" "-W" "-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
@@ -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<std::string, int, std::string> {
-    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<int>(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
@@ -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<uint16_t>((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
@@ -78,9 +78,9 @@ auto QueuedRedis<Impl>::command(Cmd cmd, Args &&...args)
 template <typename Impl>
 template <typename ...Args>
 QueuedRedis<Impl>& QueuedRedis<Impl>::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>(args)...);
+                    cmd_args.append(lcmd_name, std::forward<Args>(largs)...);
                     connection.send(cmd_args);
     };
 
@@ -95,11 +95,11 @@ auto QueuedRedis<Impl>::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
@@ -51,9 +51,9 @@ auto Redis::command(Cmd cmd, Args &&...args)
 template <typename ...Args>
 auto Redis::command(const StringView &cmd_name, Args &&...args)
     -> typename std::enable_if<!IsIter<typename LastType<Args...>::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>(args)...);
+                    cmd_args.append(lcmd_name, std::forward<Args>(largs)...);
                     connection.send(cmd_args);
     };
 
@@ -65,11 +65,11 @@ auto Redis::command(Input first, Input last)
     -> typename std::enable_if<IsIter<Input>::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
@@ -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,20 +1337,20 @@ ReplyUPtr RedisCluster::_command(Cmd cmd, const StringView &key, Args &&...args)
             SafeConnection safe_connection(*pool);
 
             return _command(cmd, safe_connection.connection(), std::forward<Args>(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();
 
             // 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>(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
@@ -47,7 +47,7 @@ template <typename T>
 struct ParseTag {};
 
 template <typename T>
-inline T parse(redisReply &reply) {
+T parse(redisReply &reply) {
     return parse(ParseTag<T>(), reply);
 }
 

diff --git 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<std::string>(*(reply->element[0]));
-    int port = reply::parse<long long>(*(reply->element[1]));
+    auto port = static_cast<int>(reply::parse<long long>(*(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<int>(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++)