Add compiler option -Wconversion and add explicit casts for conversions that may alter the value or change the sign

CMakeLists.txt

@@ -19,7 +19,11 @@ find_package(ament_cmake_ros REQUIRED)
 ament_python_install_package(${PROJECT_NAME})
 
 if(NOT WIN32)
-  add_compile_options(-Wall -Wextra -Wpedantic)
+  # About -Wno-sign-conversion: With Clang, -Wconversion implies -Wsign-conversion. There are a number of
+  # implicit sign conversions in gtest.cc, see https://ci.ros2.org/job/ci_osx/9381/clang/.
+  # Hence disabling -Wsign-conversion for now until all those have eventually been fixed.
+  # (from https://github.com/ros2/rcutils/pull/263#issuecomment-663252537)
+  add_compile_options(-Wall -Wextra -Wconversion -Wno-sign-conversion -Wpedantic)
 endif()
 
 if(WIN32)

src/cmdline_parser.c

@@ -30,7 +30,7 @@ bool rcutils_cli_option_exist(char ** begin, char ** end, const char * option)
 char * rcutils_cli_get_option(char ** begin, char ** end, const char * option)
 {
   size_t idx = 0;
-  size_t end_idx = end - begin;
+  size_t end_idx = (size_t)(end - begin);
   for (; idx < end_idx; ++idx) {
     if (strncmp(begin[idx], option, strlen(option)) == 0) {
       break;

src/error_handling.c

@@ -96,40 +96,40 @@ __format_overwriting_error_state_message(
   assert(SIZE_MAX > buffer_size);
   assert(NULL != new_error_state);
 
-  int64_t bytes_left = buffer_size;
+  int64_t bytes_left = (int64_t)buffer_size;
   do {
     char * offset = buffer;
     size_t written = 0;
 
     // write the first static part of the error message
     written = __rcutils_copy_string(
-      offset, bytes_left,
+      offset, (size_t)bytes_left,
       "\n"
       ">>> [rcutils|error_handling.c:" RCUTILS_STRINGIFY(__LINE__) "] rcutils_set_error_state()\n"
       "This error state is being overwritten:\n"
       "\n"
       "  '");
     offset += written;
-    bytes_left -= written;
+    bytes_left -= (int64_t)written;
     if (0 >= bytes_left) {break;}
 
     // write the old error string
     rcutils_error_string_t old_error_string = rcutils_get_error_string();
     written = __rcutils_copy_string(offset, sizeof(old_error_string.str), old_error_string.str);
     offset += written;
-    bytes_left -= written;
+    bytes_left -= (int64_t)written;
     if (0 >= bytes_left) {break;}
 
     // write the middle part of the state error message
     written = __rcutils_copy_string(
-      offset, bytes_left,
+      offset, (size_t)bytes_left,
       "'\n"
       "\n"
       "with this new error message:\n"
       "\n"
       "  '");
     offset += written;
-    bytes_left -= written;
+    bytes_left -= (int64_t)written;
     if (0 >= bytes_left) {break;}
 
     // format error string for new error state and write it in
@@ -139,17 +139,17 @@ __format_overwriting_error_state_message(
     __rcutils_format_error_string(&new_error_string, new_error_state);
     written = __rcutils_copy_string(offset, sizeof(new_error_string.str), new_error_string.str);
     offset += written;
-    bytes_left -= written;
+    bytes_left -= (int64_t)written;
     if (0 >= bytes_left) {break;}
 
     // write the last part of the state error message
     written = __rcutils_copy_string(
-      offset, bytes_left,
+      offset, (size_t)bytes_left,
       "'\n"
       "\n"
       "rcutils_reset_error() should be called after error handling to avoid this.\n"
       "<<<\n");
-    bytes_left -= written;
+    bytes_left -= (int64_t)written;
   } while (0);
 
 #if RCUTILS_REPORT_ERROR_HANDLING_ERRORS

src/error_handling_helpers.h

@@ -122,7 +122,7 @@ __rcutils_convert_uint64_t_into_c_str(uint64_t number, char * buffer, size_t buf
 
   // add the modulo 10 to the string and then integer divide by 10 until 0
   while (number != 0) {
-    buffer[i++] = number % 10 + '0';
+    buffer[i++] = (char)(number % 10 + '0');
     number = number / 10;
   }
 

src/format_string.c

@@ -45,8 +45,12 @@ rcutils_format_string_limit(
   va_list args2;
   va_copy(args2, args1);
   // first calculate the output string
-  size_t bytes_to_be_written = rcutils_vsnprintf(NULL, 0, format_string, args1);
+  size_t bytes_to_be_written = (size_t)rcutils_vsnprintf(NULL, 0, format_string, args1);
   va_end(args1);
+  if (bytes_to_be_written == (size_t)-1) {
+    va_end(args2);
+    return NULL;
+  }
   // allocate space for the return string
   if (bytes_to_be_written + 1 > limit) {
     bytes_to_be_written = limit - 1;

src/hash_map.c

@@ -60,8 +60,8 @@ size_t rcutils_hash_map_string_hash_func(const void * key_str)
   size_t hash = 5381;
 
   while ('\0' != *ckey_str) {
-    int c = *(ckey_str++);
-    hash = ((hash << 5) + hash) + c; /* hash * 33 + c */
+    const char c = *(ckey_str++);
+    hash = ((hash << 5) + hash) + (size_t)c; /* hash * 33 + c */
   }
 
   return hash;
@@ -177,7 +177,7 @@ static rcutils_ret_t hash_map_insert_entry(
 static rcutils_ret_t hash_map_check_and_grow_map(rcutils_hash_map_t * hash_map)
 {
   rcutils_ret_t ret = RCUTILS_RET_OK;
-  if (hash_map->impl->size >= (LOAD_FACTOR * hash_map->impl->capacity)) {
+  if (hash_map->impl->size >= (size_t)(LOAD_FACTOR * (double)hash_map->impl->capacity)) {
     size_t new_capacity = 2 * hash_map->impl->capacity;
     rcutils_array_list_t * new_map = NULL;
 

src/logging.c

@@ -322,7 +322,7 @@ rcutils_logging_severity_level_from_string(
     return RCUTILS_RET_BAD_ALLOC;
   }
   for (int i = 0; severity_string_upper[i]; ++i) {
-    severity_string_upper[i] = toupper(severity_string_upper[i]);
+    severity_string_upper[i] = (char)toupper(severity_string_upper[i]);
   }
 
   // Determine the severity value matching the severity name.

src/repl_str.c

@@ -90,11 +90,11 @@ rcutils_repl_str(
       }
     }
 
-    pos_cache[count-1] = pstr2 - str;
+    pos_cache[count-1] = (size_t)(pstr2 - str);
     pstr = pstr2 + fromlen;
   }
 
-  orglen = pstr - str + strlen(pstr);
+  orglen = (size_t)(pstr - str) + strlen(pstr);
 
   /* Allocate memory for the post-replacement string. */
   if (count > 0) {

src/time.c

@@ -59,10 +59,10 @@ rcutils_time_point_value_as_seconds_string(
   }
   // best to abs it to avoid issues with negative values in C89, see:
   //   https://stackoverflow.com/a/3604984/671658
-  uint64_t abs_time_point = llabs(*time_point);
+  uint64_t abs_time_point = (uint64_t)llabs(*time_point);
   // break into two parts to avoid floating point error
-  uint64_t seconds = abs_time_point / (1000 * 1000 * 1000);
-  uint64_t nanoseconds = abs_time_point % (1000 * 1000 * 1000);
+  uint64_t seconds = abs_time_point / (1000u * 1000u * 1000u);
+  uint64_t nanoseconds = abs_time_point % (1000u * 1000u * 1000u);
   if (
     rcutils_snprintf(
       str, str_size, "%s%.10" PRId64 ".%.9" PRId64,

src/time_unix.c

@@ -67,7 +67,7 @@ rcutils_system_time_now(rcutils_time_point_value_t * now)
     RCUTILS_SET_ERROR_MSG("unexpected negative time");
     return RCUTILS_RET_ERROR;
   }
-  *now = RCUTILS_S_TO_NS((uint64_t)timespec_now.tv_sec) + timespec_now.tv_nsec;
+  *now = RCUTILS_S_TO_NS((int64_t)timespec_now.tv_sec) + timespec_now.tv_nsec;
   return RCUTILS_RET_OK;
 }
 
@@ -98,7 +98,7 @@ rcutils_steady_time_now(rcutils_time_point_value_t * now)
     RCUTILS_SET_ERROR_MSG("unexpected negative time");
     return RCUTILS_RET_ERROR;
   }
-  *now = RCUTILS_S_TO_NS((uint64_t)timespec_now.tv_sec) + timespec_now.tv_nsec;
+  *now = RCUTILS_S_TO_NS((int64_t)timespec_now.tv_sec) + timespec_now.tv_nsec;
   return RCUTILS_RET_OK;
 }
 

test/test_char_array.cpp

@@ -61,7 +61,8 @@ TEST_F(ArrayCharTest, resize) {
   rcutils_ret_t ret = rcutils_char_array_init(&char_array, 5, &allocator);
   ASSERT_EQ(RCUTILS_RET_OK, ret);
 
-  char_array.buffer_length = snprintf(char_array.buffer, char_array.buffer_capacity, "1234") + 1;
+  char_array.buffer_length = static_cast<std::size_t>(
+    snprintf(char_array.buffer, char_array.buffer_capacity, "1234") + 1);
   EXPECT_STREQ("1234", char_array.buffer);
 
   ret = rcutils_char_array_resize(&char_array, 0);
@@ -86,8 +87,8 @@ TEST_F(ArrayCharTest, resize) {
   EXPECT_EQ(11lu, char_array.buffer_capacity);
   EXPECT_EQ(5lu, char_array.buffer_length);
 
-  char_array.buffer_length = snprintf(
-    char_array.buffer, char_array.buffer_capacity, "0987654321") + 1;
+  char_array.buffer_length = static_cast<std::size_t>(
+    snprintf(char_array.buffer, char_array.buffer_capacity, "0987654321") + 1);
   EXPECT_STREQ("0987654321", char_array.buffer);
 
   ret = rcutils_char_array_resize(&char_array, 3);

test/test_logging_macros.cpp

@@ -139,7 +139,7 @@ TEST_F(TestLoggingMacros, test_logging_function) {
 }
 
 TEST_F(TestLoggingMacros, test_logging_skipfirst) {
-  for (uint32_t i : {1, 2, 3, 4, 5}) {
+  for (uint32_t i : {1u, 2u, 3u, 4u, 5u}) {
     RCUTILS_LOG_WARN_SKIPFIRST("message %u", i);
     EXPECT_EQ(i - 1, g_log_calls);
   }

test/test_uint8_array.cpp

@@ -55,12 +55,12 @@ TEST(test_uint8_array, resize) {
   EXPECT_EQ(5u, uint8_array.buffer_length);
 
   for (uint8_t i = 0; i < 10; ++i) {
-    uint8_t u = 0xFF - i;
+    uint8_t u = static_cast<uint8_t>(0xFF - i);
     memcpy(uint8_array.buffer + i, &u, 1);
   }
   uint8_array.buffer_length = 10lu;
   for (size_t i = 0; i < uint8_array.buffer_length; ++i) {
-    uint8_t u = 0xFF - static_cast<uint8_t>(i);
+    uint8_t u = static_cast<uint8_t>(0xFF - i);
     EXPECT_EQ(u, uint8_array.buffer[i]);
   }