Converge the warning-flags in Makefile and CMake builds (valkey-io#67)

Enable the same warnings in CMake as used in the Makefile. Enable `-pedantic` in Makefile, as in CMake, to `issue all the warnings demanded by strict ISO C`. Includes fixes of warnings: - Correcting -Wwrite-strings warnings in cluster tests (warns about the deprecated conversion from string constants to `char*`). - Use the correct libevent2 type `evutil_socket_t` instead of `int` when implementing the libevent2 callback function in our tests. Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
bjosv · Aug 14, 2024 · ba31923 · ba31923
1 parent 9188dbd
commit ba31923
Show file tree

Hide file tree

Showing 14 changed files with 35 additions and 39 deletions.
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -22,8 +22,6 @@ jobs:
       - name: Build using cmake
         env:
           EXTRA_CMAKE_OPTS: -DENABLE_EXAMPLES:BOOL=ON -DENABLE_SSL:BOOL=ON
-          CFLAGS: -Werror
-          CXXFLAGS: -Werror
         run: mkdir build && cd build && cmake .. && make
 
       - name: Build using makefile
@@ -57,8 +55,6 @@ jobs:
       - name: Build using cmake
         env:
           EXTRA_CMAKE_OPTS: -DENABLE_EXAMPLES:BOOL=ON -DENABLE_SSL:BOOL=ON
-          CFLAGS: -Werror
-          CXXFLAGS: -Werror
         run: mkdir build && cd build && cmake3 .. && make
 
       - name: Build using Makefile
@@ -102,8 +98,6 @@ jobs:
       - name: Build using cmake
         env:
           EXTRA_CMAKE_OPTS: -DENABLE_EXAMPLES:BOOL=ON -DENABLE_SSL:BOOL=ON
-          CFLAGS: -Werror
-          CXXFLAGS: -Werror
         run: mkdir build && cd build && cmake .. && make
 
       - name: Build using Makefile

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -29,6 +29,14 @@ OPTION(ENABLE_RDMA "Build valkey_rdma for RDMA support" OFF)
 SET(CMAKE_C_STANDARD 99)
 SET(CMAKE_DEBUG_POSTFIX d)
 
+# Set target-common flags
+if(NOT WIN32)
+  add_compile_options(-Werror -Wall -Wextra -pedantic)
+  add_compile_options(-Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers)
+else()
+  add_definitions(-D_CRT_SECURE_NO_WARNINGS -DWIN32_LEAN_AND_MEAN)
+endif()
+
 SET(valkey_sources
     src/adlist.c
     src/alloc.c
@@ -45,10 +53,6 @@ SET(valkey_sources
     src/valkeycluster.c
     src/vkutil.c)
 
-IF(WIN32)
-    ADD_DEFINITIONS(-D_CRT_SECURE_NO_WARNINGS -DWIN32_LEAN_AND_MEAN)
-ENDIF()
-
 ADD_LIBRARY(valkey ${valkey_sources})
 ADD_LIBRARY(valkey::valkey ALIAS valkey)
 set(valkey_export_name valkey CACHE STRING "Name of the exported target")

diff --git a/Makefile b/Makefile
@@ -60,7 +60,7 @@ export VALKEY_TEST_CONFIG
 CC := $(if $(shell command -v $(firstword $(CC)) >/dev/null 2>&1 && echo OK),$(CC),gcc)
 
 OPTIMIZATION?=-O3
-WARNINGS=-Wall -Wextra -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers
+WARNINGS=-Wall -Wextra -pedantic -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers
 USE_WERROR?=1
 ifeq ($(USE_WERROR),1)
   WARNINGS+=-Werror

diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
@@ -53,12 +53,10 @@ find_path(LIBEVENT_INCLUDES event2/event.h)
 if(LIBEVENT_INCLUDES)
   include_directories(${LIBEVENT_INCLUDES})
 endif()
-
 if(MSVC OR MINGW)
   find_library(LIBEVENT_LIBRARY Libevent)
 else()
-  add_compile_options(-Wall -Wextra -pedantic -Werror)
-  # Debug mode for tests
+  # Use the Debug configuration when building tests (no -DNDEBUG)
   set(CMAKE_BUILD_TYPE "Debug" CACHE STRING "" FORCE)
 endif()
 

diff --git a/tests/client_test.c b/tests/client_test.c
@@ -1583,7 +1583,7 @@ void async_disconnect(valkeyAsyncContext *ac) {
 }
 
 /* Testcase timeout, will trigger a failure */
-void timeout_cb(int fd, short event, void *arg) {
+void timeout_cb(evutil_socket_t fd, short event, void *arg) {
     (void) fd; (void) event; (void) arg;
     printf("Timeout in async testing!\n");
     exit(1);

diff --git a/tests/clusterclient.c b/tests/clusterclient.c
@@ -43,7 +43,7 @@ void printReply(const valkeyReply *reply) {
 void eventCallback(const valkeyClusterContext *cc, int event, void *privdata) {
     (void)cc;
     (void)privdata;
-    char *e = NULL;
+    const char *e;
     switch (event) {
     case VALKEYCLUSTER_EVENT_SLOTMAP_UPDATED:
         e = "slotmap-updated";

diff --git a/tests/clusterclient_async.c b/tests/clusterclient_async.c
@@ -54,7 +54,7 @@ int num_running = 0;
 int resend_failed_cmd = 0;
 int send_to_all = 0;
 
-void sendNextCommand(int, short, void *);
+void sendNextCommand(evutil_socket_t, short, void *);
 
 void printReply(const valkeyReply *reply) {
     switch (reply->type) {
@@ -102,7 +102,7 @@ void replyCallback(valkeyClusterAsyncContext *acc, void *r, void *privdata) {
     }
 }
 
-void sendNextCommand(int fd, short kind, void *arg) {
+void sendNextCommand(evutil_socket_t fd, short kind, void *arg) {
     UNUSED(fd);
     UNUSED(kind);
     valkeyClusterAsyncContext *acc = arg;

diff --git a/tests/clusterclient_reconnect_async.c b/tests/clusterclient_reconnect_async.c
@@ -20,7 +20,7 @@
 /* Unfortunately there is no error code for this error to match */
 #define VALKEY_ENOCLUSTER "ERR This instance has cluster support disabled"
 
-void sendNextCommand(int, short, void *);
+void sendNextCommand(evutil_socket_t, short, void *);
 
 void connectToValkey(valkeyClusterAsyncContext *acc) {
     /* reset context in case of reconnect */
@@ -61,7 +61,7 @@ void replyCallback(valkeyClusterAsyncContext *acc, void *r, void *privdata) {
     event_base_once(acc->adapter, -1, EV_TIMEOUT, sendNextCommand, acc, NULL);
 }
 
-void sendNextCommand(int fd, short kind, void *arg) {
+void sendNextCommand(evutil_socket_t fd, short kind, void *arg) {
     UNUSED(fd);
     UNUSED(kind);
     valkeyClusterAsyncContext *acc = arg;

diff --git a/tests/ct_async_glib.c b/tests/ct_async_glib.c
@@ -53,10 +53,10 @@ int main(int argc, char **argv) {
     valkeyClusterAsyncSetConnectCallback(acc, connectCallback);
     valkeyClusterAsyncSetDisconnectCallback(acc, disconnectCallback);
 
-    status = valkeyClusterAsyncCommand(acc, setCallback, "id", "SET key value");
+    status = valkeyClusterAsyncCommand(acc, setCallback, (char*)"id", "SET key value");
     ASSERT_MSG(status == VALKEY_OK, acc->errstr);
 
-    status = valkeyClusterAsyncCommand(acc, getCallback, "id", "GET key");
+    status = valkeyClusterAsyncCommand(acc, getCallback, (char*)"id", "GET key");
     ASSERT_MSG(status == VALKEY_OK, acc->errstr);
 
     g_main_loop_run(mainloop);

diff --git a/tests/ct_connection.c b/tests/ct_connection.c
@@ -306,10 +306,10 @@ void test_command_timeout_set_while_connected(void) {
 //------------------------------------------------------------------------------
 typedef struct ExpectedResult {
     int type;
-    char *str;
+    const char *str;
     bool disconnect;
     bool noreply;
-    char *errstr;
+    const char *errstr;
 } ExpectedResult;
 
 // Callback for Valkey connects and disconnects

diff --git a/tests/ct_out_of_memory_handling.c b/tests/ct_out_of_memory_handling.c
@@ -202,7 +202,7 @@ void test_alloc_failure_handling(void) {
         valkeyReply *reply;
         const char *cmd = "SET key value";
 
-        valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, "key");
+        valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char*)"key");
         assert(node);
 
         // OOM failing commands
@@ -265,7 +265,7 @@ void test_alloc_failure_handling(void) {
         valkeyReply *reply;
         const char *cmd = "SET foo one";
 
-        valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, "foo");
+        valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char*)"foo");
         assert(node);
 
         // OOM failing appends
@@ -313,8 +313,8 @@ void test_alloc_failure_handling(void) {
         prepare_allocation_test(cc, 1000);
 
         /* Get the source information for the migration. */
-        unsigned int slot = valkeyClusterGetSlotByKey("foo");
-        valkeyClusterNode *srcNode = valkeyClusterGetNodeByKey(cc, "foo");
+        unsigned int slot = valkeyClusterGetSlotByKey((char*)"foo");
+        valkeyClusterNode *srcNode = valkeyClusterGetNodeByKey(cc, (char*)"foo");
         int srcPort = srcNode->port;
 
         /* Get a destination node to migrate the slot to. */
@@ -371,7 +371,7 @@ void test_alloc_failure_handling(void) {
          * allowing a high number of allocations. */
         prepare_allocation_test(cc, 1000);
         /* Fetch the nodes again, in case the slotmap has been reloaded. */
-        srcNode = valkeyClusterGetNodeByKey(cc, "foo");
+        srcNode = valkeyClusterGetNodeByKey(cc, (char*)"foo");
         dstNode = getNodeByPort(cc, dstPort);
         reply = valkeyClusterCommandToNode(
             cc, srcNode, "CLUSTER SETSLOT %d NODE %s", slot, replyDstId->str);
@@ -442,7 +442,7 @@ void test_alloc_failure_handling(void) {
 
 typedef struct ExpectedResult {
     int type;
-    char *str;
+    const char *str;
     bool disconnect;
 } ExpectedResult;
 

diff --git a/tests/ct_pipeline.c b/tests/ct_pipeline.c
@@ -63,7 +63,7 @@ void test_pipeline(void) {
 
 typedef struct ExpectedResult {
     int type;
-    char *str;
+    const char *str;
     bool disconnect;
 } ExpectedResult;
 

diff --git a/tests/ct_specific_nodes.c b/tests/ct_specific_nodes.c
@@ -45,7 +45,7 @@ void test_command_to_all_nodes(valkeyClusterContext *cc) {
 
 void test_transaction(valkeyClusterContext *cc) {
 
-    valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, "foo");
+    valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char*)"foo");
     assert(node);
 
     valkeyReply *reply;
@@ -73,7 +73,7 @@ void test_streams(valkeyClusterContext *cc) {
     char *id;
 
     /* Get the node that handles given stream */
-    valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, "mystream");
+    valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char*)"mystream");
     assert(node);
 
     /* Preparation: remove old stream/key */
@@ -82,7 +82,7 @@ void test_streams(valkeyClusterContext *cc) {
     freeReplyObject(reply);
 
     /* Query wrong node */
-    valkeyClusterNode *wrongNode = valkeyClusterGetNodeByKey(cc, "otherstream");
+    valkeyClusterNode *wrongNode = valkeyClusterGetNodeByKey(cc, (char*)"otherstream");
     assert(node != wrongNode);
     reply = valkeyClusterCommandToNode(cc, wrongNode, "XLEN mystream");
     CHECK_REPLY_ERROR(cc, reply, "MOVED");
@@ -235,7 +235,7 @@ void test_pipeline_transaction(valkeyClusterContext *cc) {
     int status;
     valkeyReply *reply;
 
-    valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, "foo");
+    valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char*)"foo");
     assert(node);
 
     status = valkeyClusterAppendCommandToNode(cc, node, "MULTI");
@@ -274,11 +274,11 @@ void test_pipeline_transaction(valkeyClusterContext *cc) {
 //------------------------------------------------------------------------------
 typedef struct ExpectedResult {
     int type;
-    char *str;
+    const char *str;
     size_t elements;
     bool disconnect;
     bool noreply;
-    char *errstr;
+    const char *errstr;
 } ExpectedResult;
 
 // Callback for Valkey connects and disconnects
@@ -487,7 +487,7 @@ void test_async_transaction(void) {
     status = valkeyClusterLibeventAttach(acc, base);
     assert(status == VALKEY_OK);
 
-    valkeyClusterNode *node = valkeyClusterGetNodeByKey(acc->cc, "foo");
+    valkeyClusterNode *node = valkeyClusterGetNodeByKey(acc->cc, (char*)"foo");
     assert(node);
 
     ExpectedResult r1 = {.type = VALKEY_REPLY_STATUS, .str = "OK"};

diff --git a/tests/ut_parse_cmd.c b/tests/ut_parse_cmd.c
@@ -10,7 +10,7 @@
 #include <string.h>
 
 /* Helper for the macro ASSERT_KEY below. */
-void check_key(char *key, struct cmd *command, char *file, int line) {
+void check_key(const char *key, struct cmd *command, const char *file, int line) {
     if (command->result != CMD_PARSE_OK) {
         fprintf(stderr, "%s:%d: Command parsing failed: %s\n", file, line,
                 command->errstr);