On 11/6/20 7:33 AM, Markus Armbruster wrote:
> Thomas Huth <th...@redhat.com> writes:
>
>> On 05/11/2020 06.14, AlexChen wrote:
>>> On 2020/11/4 18:44, Thomas Huth wrote:
>>>> On 04/11/2020 11.23, AlexChen wrote:
>>>>> We should use printf format specifier "%u" instead of "%d" for
>>>>> argument of type "unsigned int".
>>>>>
>>>>> Reported-by: Euler Robot <euler.ro...@huawei.com>
>>>>> Signed-off-by: Alex Chen <alex.c...@huawei.com>
>>>>> ---
>>>>> tests/qtest/arm-cpu-features.c | 8 ++++----
>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/tests/qtest/arm-cpu-features.c
>>>>> b/tests/qtest/arm-cpu-features.c
>>>>> index d20094d5a7..bc681a95d5 100644
>>>>> --- a/tests/qtest/arm-cpu-features.c
>>>>> +++ b/tests/qtest/arm-cpu-features.c
>>>>> @@ -536,7 +536,7 @@ static void test_query_cpu_model_expansion_kvm(const
>>>>> void *data)
>>>>> if (kvm_supports_sve) {
>>>>> g_assert(vls != 0);
>>>>> max_vq = 64 - __builtin_clzll(vls);
>>>>> - sprintf(max_name, "sve%d", max_vq * 128);
>>>>> + sprintf(max_name, "sve%u", max_vq * 128);
>>>>>
>>>>> /* Enabling a supported length is of course fine. */
>>>>> assert_sve_vls(qts, "host", vls, "{ %s: true }", max_name);
>>>>> @@ -556,7 +556,7 @@ static void test_query_cpu_model_expansion_kvm(const
>>>>> void *data)
>>>>> * unless all larger, supported vector lengths are also
>>>>> * disabled.
>>>>> */
>>>>> - sprintf(name, "sve%d", vq * 128);
>>>>> + sprintf(name, "sve%u", vq * 128);
>>>>> error = g_strdup_printf("cannot disable %s", name);
>>>>> assert_error(qts, "host", error,
>>>>> "{ %s: true, %s: false }",
>>>>> @@ -569,7 +569,7 @@ static void test_query_cpu_model_expansion_kvm(const
>>>>> void *data)
>>>>> * we need at least one vector length enabled.
>>>>> */
>>>>> vq = __builtin_ffsll(vls);
>>>>> - sprintf(name, "sve%d", vq * 128);
>>>>> + sprintf(name, "sve%u", vq * 128);
>>>>> error = g_strdup_printf("cannot disable %s", name);
>>>>> assert_error(qts, "host", error, "{ %s: false }", name);
>>>>> g_free(error);
>>>>> @@ -581,7 +581,7 @@ static void test_query_cpu_model_expansion_kvm(const
>>>>> void *data)
>>>>> }
>>>>> }
>>>>> if (vq <= SVE_MAX_VQ) {
>>>>> - sprintf(name, "sve%d", vq * 128);
>>>>> + sprintf(name, "sve%u", vq * 128);
>>>>> error = g_strdup_printf("cannot enable %s", name);
>>>>> assert_error(qts, "host", error, "{ %s: true }", name);
>>>>> g_free(error);
>>>>>
>>>>
>>>> max_vq and vq are both "uint32_t" and not "unsigned int" ... so if you want
>>>> to fix this really really correctly, please use PRIu32 from inttypes.h
>>>> instead.
>>>>
>>>
>>> Hi Thomas,
>>> Thanks for your review.
>>> According to the definition of the macro PRIu32(# define PRIu32
>>> "u"),
>>> using PRIu32 works the same as using %u to print, and using PRIu32 to print
>>> is relatively rare in QEMU(%u 720, PRIu32 only 120). Can we continue to use
>>> %u to
>>> print max_vq and vq in this patch.
>>> Of course, this is just my small small suggestion. If you think it is
>>> better to use
>>> PRIu32 for printing, I will send patch V2.
>>
>> Well, %u happens to work since "int" is 32-bit with all current compilers
>> that we support.
>
> Yes, it works.
>
>> But if there is ever a compiler where the size of int is
>> different, you'll get a compiler warning here again.
>
> No, we won't.
>
> If we ever use a compiler where int is narrower than 32 bits, then the
> type of the argument is actually uint32_t[1]. We can forget about this
> case, because "int narrower than 32 bits" is not going to fly with our
> code base.
>
> If we ever use a compiler where int is wider than 32 bits, then the type
> of the argument is *not* uint32_t[2]. PRIu32 will work anyway, because
> it will actually retrieve an unsigned int argument, *not* an uint32_t
> argument[3].
>
> In other words "%" PRIu32 is just a less legible alias for "%u" in all
> cases that matter.
Can we add a checkpatch rule to avoid using 'PRI[dux]32' format,
so it is clear for everyone?
>
> And that's why I would simply use "%u".
>
>> So if we now fix this
>> up, then let's do it really right and use PRIu32, please.
>>
>> Thomas
>
>
> [1] Because promotion does nothing either argument, and the usual
> arithmetic conversions convert to uint32_t. See my first reply.
>
> [2] Because uint32_t gets promoted to unsigned int. See my first reply.
>
> [3] Because variable arguments undergo default argument promotion (§
> 6.5.2.2 Function calls), which promotes uint32_t to unsigned int.
>
>
