From 9a5c2d284d63db84e81893146a04356246f6941b Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Fri, 26 Jun 2020 01:08:13 +0200 Subject: [PATCH 1/2] Add compiler option -Wconversion and add explicit casts for conversions that may alter the value or change the sign Signed-off-by: Johannes Meyer --- CMakeLists.txt | 2 +- src/cmdline_parser.c | 2 +- src/error_handling.c | 18 +++++++++--------- src/error_handling_helpers.h | 2 +- src/format_string.c | 6 +++++- src/hash_map.c | 6 +++--- src/logging.c | 2 +- src/repl_str.c | 4 ++-- src/time.c | 6 +++--- src/time_unix.c | 4 ++-- test/test_uint8_array.cpp | 4 ++-- 11 files changed, 30 insertions(+), 26 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 0f7429cf..8e849f72 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -19,7 +19,7 @@ find_package(ament_cmake_ros REQUIRED) ament_python_install_package(${PROJECT_NAME}) if(NOT WIN32) - add_compile_options(-Wall -Wextra -Wpedantic) + add_compile_options(-Wall -Wextra -Wpedantic -Wconversion) endif() if(WIN32) diff --git a/src/cmdline_parser.c b/src/cmdline_parser.c index b1623e20..148c26f1 100644 --- a/src/cmdline_parser.c +++ b/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; diff --git a/src/error_handling.c b/src/error_handling.c index 868a5019..a0605373 100644 --- a/src/error_handling.c +++ b/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 diff --git a/src/error_handling_helpers.h b/src/error_handling_helpers.h index a0e898f9..70acb4e8 100644 --- a/src/error_handling_helpers.h +++ b/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; } diff --git a/src/format_string.c b/src/format_string.c index 4dbac918..1f8a6a81 100644 --- a/src/format_string.c +++ b/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; diff --git a/src/hash_map.c b/src/hash_map.c index ccfe2e52..e6b4ee84 100644 --- a/src/hash_map.c +++ b/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; diff --git a/src/logging.c b/src/logging.c index f3c29ca6..b81df173 100644 --- a/src/logging.c +++ b/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. diff --git a/src/repl_str.c b/src/repl_str.c index 1ebb7b81..c908f6d8 100644 --- a/src/repl_str.c +++ b/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) { diff --git a/src/time.c b/src/time.c index 6b6517f2..c1b20878 100644 --- a/src/time.c +++ b/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, diff --git a/src/time_unix.c b/src/time_unix.c index 028fb453..47c996c8 100644 --- a/src/time_unix.c +++ b/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; } diff --git a/test/test_uint8_array.cpp b/test/test_uint8_array.cpp index 035c2c5e..f30791c7 100644 --- a/test/test_uint8_array.cpp +++ b/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(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(i); + uint8_t u = static_cast(0xFF - i); EXPECT_EQ(u, uint8_array.buffer[i]); } From 7db43f8ce14aed9b5d72c6f60f18e11214bc10c5 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Fri, 24 Jul 2020 00:14:06 +0200 Subject: [PATCH 2/2] Fix or ignore sign conversion warnings which affect macOS builds with Clang See https://github.com/ros2/rcutils/pull/263#issuecomment-663252537. Signed-off-by: Johannes Meyer --- CMakeLists.txt | 6 +++++- test/test_char_array.cpp | 7 ++++--- test/test_logging_macros.cpp | 2 +- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 8e849f72..a73082ad 100644 --- a/CMakeLists.txt +++ b/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 -Wconversion) + # 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) diff --git a/test/test_char_array.cpp b/test/test_char_array.cpp index 05012d8b..7c948105 100644 --- a/test/test_char_array.cpp +++ b/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( + 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( + snprintf(char_array.buffer, char_array.buffer_capacity, "0987654321") + 1); EXPECT_STREQ("0987654321", char_array.buffer); ret = rcutils_char_array_resize(&char_array, 3); diff --git a/test/test_logging_macros.cpp b/test/test_logging_macros.cpp index 825fc851..b193ed5e 100644 --- a/test/test_logging_macros.cpp +++ b/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); }