From 14a8002ae5f4e4e8987228dc2739ffabc2e036ec Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 14 Mar 2022 11:53:02 +0100 Subject: [PATCH 1/5] test: change // comments to /* */ We use // comments only for "FIXME" style comments, and explanatory comments get /* */, as per coding style. --- src/test/test-macro.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/test-macro.c b/src/test/test-macro.c index d33d22c18c..24411f10a5 100644 --- a/src/test/test-macro.c +++ b/src/test/test-macro.c @@ -340,7 +340,7 @@ TEST(flags) { assert_se(!FLAGS_SET(F1 | F2, F1 | F3)); assert_se(!FLAGS_SET(F1 | F2 | F3, ~F_ALL)); - // Check for no double eval. + /* Check for no double eval. */ n = F2; f = F1; assert_se(!FLAGS_SET(--n, ++f)); @@ -379,7 +379,7 @@ TEST(flags) { assert_se(UPDATE_FLAG(F1, F_ALL, false) == 0); assert_se(UPDATE_FLAG(F_ALL, F_ALL, false) == 0); - // Check for no double eval. + /* Check for no double eval. */ n = F2; f = F1; assert_se(UPDATE_FLAG(--n, ++f, true) == (F1 | F2)); From 92463840f804be31332fba65b60c1533500f0974 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 14 Mar 2022 12:01:47 +0100 Subject: [PATCH 2/5] macro: DECIMAL_STR_WIDTH() is about *values* not *types* Hence, check if the value is negative, not whether the type can carry negatives. Follow-up for: e3dd9ea8ea4510221f73071ad30ee657ca77565d --- src/basic/macro.h | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/basic/macro.h b/src/basic/macro.h index 9e62f9c71c..214654386c 100644 --- a/src/basic/macro.h +++ b/src/basic/macro.h @@ -319,12 +319,22 @@ static inline int __coverity_check_and_return__(int condition) { sizeof(type) <= 4 ? 10U : \ sizeof(type) <= 8 ? 20U : sizeof(int[-2*(sizeof(type) > 8)]))) +/* Returns the number of chars needed to format the specified integer value. It's hence more specific than + * DECIMAL_STR_MAX() which answers the same question for all possible values of the specified type. Does + * *not* include space for a trailing NUL. (If you wonder why we special case _x_ == 0 here: it's to trick + * out gcc's -Wtype-limits, which would complain on comparing an unsigned type with < 0, otherwise. By + * special-casing == 0 here first, we can use <= 0 instead of < 0 to trick out gcc.) */ #define DECIMAL_STR_WIDTH(x) \ ({ \ typeof(x) _x_ = (x); \ - size_t ans = IS_SIGNED_INTEGER_TYPE(_x_) ? 2 : 1; \ - while ((_x_ /= 10) != 0) \ - ans++; \ + size_t ans; \ + if (_x_ == 0) \ + ans = 1; \ + else { \ + ans = _x_ <= 0 ? 2 : 1; \ + while ((_x_ /= 10) != 0) \ + ans++; \ + } \ ans; \ }) From 845ed472315e5e4ac6960d809ee03ad841fa14ec Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 14 Mar 2022 12:02:42 +0100 Subject: [PATCH 3/5] test: add test for DECIMAL_STR_WIDTH() --- src/test/test-macro.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/test/test-macro.c b/src/test/test-macro.c index 24411f10a5..84b0af6870 100644 --- a/src/test/test-macro.c +++ b/src/test/test-macro.c @@ -387,4 +387,27 @@ TEST(flags) { assert_se(f == F2); } +TEST(DECIMAL_STR_WIDTH) { + assert_se(DECIMAL_STR_WIDTH(0) == 1); + assert_se(DECIMAL_STR_WIDTH(1) == 1); + assert_se(DECIMAL_STR_WIDTH(2) == 1); + assert_se(DECIMAL_STR_WIDTH(9) == 1); + assert_se(DECIMAL_STR_WIDTH(10) == 2); + assert_se(DECIMAL_STR_WIDTH(11) == 2); + assert_se(DECIMAL_STR_WIDTH(99) == 2); + assert_se(DECIMAL_STR_WIDTH(100) == 3); + assert_se(DECIMAL_STR_WIDTH(101) == 3); + assert_se(DECIMAL_STR_WIDTH(-1) == 2); + assert_se(DECIMAL_STR_WIDTH(-2) == 2); + assert_se(DECIMAL_STR_WIDTH(-9) == 2); + assert_se(DECIMAL_STR_WIDTH(-10) == 3); + assert_se(DECIMAL_STR_WIDTH(-11) == 3); + assert_se(DECIMAL_STR_WIDTH(-99) == 3); + assert_se(DECIMAL_STR_WIDTH(-100) == 4); + assert_se(DECIMAL_STR_WIDTH(-101) == 4); + assert_se(DECIMAL_STR_WIDTH(UINT64_MAX) == STRLEN("18446744073709551615")); + assert_se(DECIMAL_STR_WIDTH(INT64_MAX) == STRLEN("9223372036854775807")); + assert_se(DECIMAL_STR_WIDTH(INT64_MIN) == STRLEN("-9223372036854775808")); +} + DEFINE_TEST_MAIN(LOG_INFO); From 56da8d5af37bda6950f058066fa05c5fdcc20f6e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 14 Mar 2022 14:44:54 +0100 Subject: [PATCH 4/5] macro: handle DECIMAL_STR_MAX() special cases more accurately MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit So far DECIMAL_STR_MAX() overestimated the types in two ways: it would also adds space for a "-" for unsigned types. And it would always return the same size for 64bit values regardless of signedness, even though the longest maximum numbers for signed and unsigned differ in length by one digit. i.e. 2^64-1 (i.e. UINT64_MAX) is one decimal digit longer than -2^63 (INT64_MIN) - for the other integer widths the number of digits in the "longest" decimal value is always the same, regardless of signedness. by example: strlen("65535") == strlen("32768") (i.e. the relevant 16 bit limits) holds — and similar for 8bit and 32bit integer width limits — but strlen("18446744073709551615") > strlen("9223372036854775808") (i.e. the relevant 64 bit limits). Let's fix both misestimations. --- src/basic/macro.h | 11 +++++------ src/test/test-format-util.c | 10 ++++++---- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/basic/macro.h b/src/basic/macro.h index 214654386c..fb1ae65d28 100644 --- a/src/basic/macro.h +++ b/src/basic/macro.h @@ -309,15 +309,14 @@ static inline int __coverity_check_and_return__(int condition) { #define sizeof_field(struct_type, member) sizeof(((struct_type *) 0)->member) -/* Returns the number of chars needed to format variables of the - * specified type as a decimal string. Adds in extra space for a - * negative '-' prefix (hence works correctly on signed - * types). Includes space for the trailing NUL. */ +/* Returns the number of chars needed to format variables of the specified type as a decimal string. Adds in + * extra space for a negative '-' prefix for signed types. Includes space for the trailing NUL. */ #define DECIMAL_STR_MAX(type) \ - (2U+(sizeof(type) <= 1 ? 3U : \ + ((size_t) IS_SIGNED_INTEGER_TYPE(type) + 1U + \ + (sizeof(type) <= 1 ? 3U : \ sizeof(type) <= 2 ? 5U : \ sizeof(type) <= 4 ? 10U : \ - sizeof(type) <= 8 ? 20U : sizeof(int[-2*(sizeof(type) > 8)]))) + sizeof(type) <= 8 ? (IS_SIGNED_INTEGER_TYPE(type) ? 19U : 20U) : sizeof(int[-2*(sizeof(type) > 8)]))) /* Returns the number of chars needed to format the specified integer value. It's hence more specific than * DECIMAL_STR_MAX() which answers the same question for all possible values of the specified type. Does diff --git a/src/test/test-format-util.c b/src/test/test-format-util.c index 3e640a84b3..ee74d1c898 100644 --- a/src/test/test-format-util.c +++ b/src/test/test-format-util.c @@ -10,10 +10,12 @@ assert_cc(STRLEN("xxx") == 3); assert_cc(STRLEN("") == 0); assert_cc(STRLEN(L"xxx") == 3 * sizeof(wchar_t)); assert_cc(STRLEN(L"") == 0); -assert_cc(DECIMAL_STR_MAX(uint8_t) == 5); -assert_cc(DECIMAL_STR_MAX(int8_t) == 5); -assert_cc(DECIMAL_STR_MAX(uint64_t) == 22); -assert_cc(DECIMAL_STR_MAX(char) == 5); +assert_cc(DECIMAL_STR_MAX(uint8_t) == STRLEN("255")+1); +assert_cc(DECIMAL_STR_MAX(int8_t) == STRLEN("-127")+1); +assert_cc(DECIMAL_STR_MAX(uint64_t) == STRLEN("18446744073709551615")+1); +assert_cc(DECIMAL_STR_MAX(int64_t) == CONST_MAX(STRLEN("-9223372036854775808"), STRLEN("9223372036854775807"))+1); +assert_cc(DECIMAL_STR_MAX(signed char) == STRLEN("-127")+1); +assert_cc(DECIMAL_STR_MAX(unsigned char) == STRLEN("255")+1); assert_cc(CONST_MAX(DECIMAL_STR_MAX(int8_t), STRLEN("xxx")) == 5); static void test_format_bytes_one(uint64_t val, bool trailing_B, const char *iec_with_p, const char *iec_without_p, From b068ef6b3ee42dc9bfc54f6aec7087911966022e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 14 Mar 2022 14:48:17 +0100 Subject: [PATCH 5/5] test: add test case for DECIMAL_STR_MAX() --- src/test/test-macro.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/test/test-macro.c b/src/test/test-macro.c index 84b0af6870..960ff4bc09 100644 --- a/src/test/test-macro.c +++ b/src/test/test-macro.c @@ -410,4 +410,27 @@ TEST(DECIMAL_STR_WIDTH) { assert_se(DECIMAL_STR_WIDTH(INT64_MIN) == STRLEN("-9223372036854775808")); } +TEST(DECIMAL_STR_MAX) { + int8_t s8_longest = INT8_MIN; + int16_t s16_longest = INT16_MIN; + int32_t s32_longest = INT32_MIN; + int64_t s64_longest = INT64_MIN; + uint8_t u8_longest = UINT8_MAX; + uint16_t u16_longest = UINT16_MAX; + uint32_t u32_longest = UINT32_MAX; + uint64_t u64_longest = UINT64_MAX; + + /* NB: Always add +1, because DECIMAL_STR_MAX() includes space for trailing NUL byte, but + * DECIMAL_STR_WIDTH() does not! */ + assert_se(DECIMAL_STR_MAX(int8_t) == DECIMAL_STR_WIDTH(s8_longest)+1); + assert_se(DECIMAL_STR_MAX(int16_t) == DECIMAL_STR_WIDTH(s16_longest)+1); + assert_se(DECIMAL_STR_MAX(int32_t) == DECIMAL_STR_WIDTH(s32_longest)+1); + assert_se(DECIMAL_STR_MAX(int64_t) == DECIMAL_STR_WIDTH(s64_longest)+1); + + assert_se(DECIMAL_STR_MAX(uint8_t) == DECIMAL_STR_WIDTH(u8_longest)+1); + assert_se(DECIMAL_STR_MAX(uint16_t) == DECIMAL_STR_WIDTH(u16_longest)+1); + assert_se(DECIMAL_STR_MAX(uint32_t) == DECIMAL_STR_WIDTH(u32_longest)+1); + assert_se(DECIMAL_STR_MAX(uint64_t) == DECIMAL_STR_WIDTH(u64_longest)+1); +} + DEFINE_TEST_MAIN(LOG_INFO);