Re: [libav-devel] [PATCH] cfhd: Use proper ISO C printf conversion specifiers

[Mark Thompson]

On 04/04/17 17:35, Vittorio Giovara wrote:
> On Tue, Apr 4, 2017 at 6:24 PM, Diego Biurrun  wrote:
>> On Tue, Apr 04, 2017 at 05:47:35PM +0200, Vittorio Giovara wrote:
>>> On Tue, Apr 4, 2017 at 5:10 PM, Diego Biurrun  wrote:
 On Tue, Apr 04, 2017 at 04:39:23PM +0200, Vittorio Giovara wrote:
> libavcodec/cfhd.c:258:16: warning: format specifies
>   type 'unsigned short' but the argument has type 'int' [-Wformat]
> --- a/libavcodec/cfhd.c
> +++ b/libavcodec/cfhd.c
> @@ -254,11 +254,11 @@ static int parse_tag(CFHDContext *s, GetByteContext 
> *gb,
>
>  if (abs_tag8 >= 0x60 && abs_tag8 <= 0x6F) {
> -av_log(s->avctx, AV_LOG_DEBUG, "large len %"PRIX16"\n",
> +av_log(s->avctx, AV_LOG_DEBUG, "large len %"PRIX32"\n",
> ((tagu & 0xFF) << 16) | data);

 The warning does not match your change. tagu is uint16_t. I don't remember
 what it gets promoted to offhand, but probably not uint32_t.
>>>
>>> it gets promoted to 'int'
>>
>> So use the correct conversion specifier for 'int'.
> 
> ok so a normal %X should do

No.

The whole point of the %"PRI..." conversion specifiers is that they expand to 
convert the promoted argument, because vararg functions always promote the 
arguments.  In this case that is indeed int, but the PRIx16 is already taking 
that into account (look in inttypes.h on your machine with int wider than 16 
bits and see #define PRIx16 "x").

Suppose we are doing arithmetic combining some ints and intfoo_ts, in order to 
pass that as a vararg argument to barf (some printf-like function).  The result 
will be either:

* int, because the promotion rank of intfoo_t is less than or equal to that of 
int.
  In this case both %d (int directly) and %"PRIdFOO" (also int, because 
intfoo_t is promoted) are correct.

* intfoo_t, because the promotion rank of intfoo_t is greater than that of int.
  In this case only %"PRIdFOO" is correct, and %d is definitely wrong because 
the argument is wider than that.

While the warning isn't necessarily harmful when you definitely know that int 
is wider than intfoo_t on every platform you will ever support, it would be 
wrong to make the change if you can't absolutely guarantee that - the warning 
seems to be encouraging nonportable platform-specific behaviour.  It would be 
better to complain to the compiler writers to not generate this warning.

- Mark
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] cfhd: Use proper ISO C printf conversion specifiers

[Vittorio Giovara]

On Tue, Apr 4, 2017 at 6:24 PM, Diego Biurrun  wrote:
> On Tue, Apr 04, 2017 at 05:47:35PM +0200, Vittorio Giovara wrote:
>> On Tue, Apr 4, 2017 at 5:10 PM, Diego Biurrun  wrote:
>> > On Tue, Apr 04, 2017 at 04:39:23PM +0200, Vittorio Giovara wrote:
>> >> libavcodec/cfhd.c:258:16: warning: format specifies
>> >>   type 'unsigned short' but the argument has type 'int' [-Wformat]
>> >> --- a/libavcodec/cfhd.c
>> >> +++ b/libavcodec/cfhd.c
>> >> @@ -254,11 +254,11 @@ static int parse_tag(CFHDContext *s, GetByteContext 
>> >> *gb,
>> >>
>> >>  if (abs_tag8 >= 0x60 && abs_tag8 <= 0x6F) {
>> >> -av_log(s->avctx, AV_LOG_DEBUG, "large len %"PRIX16"\n",
>> >> +av_log(s->avctx, AV_LOG_DEBUG, "large len %"PRIX32"\n",
>> >> ((tagu & 0xFF) << 16) | data);
>> >
>> > The warning does not match your change. tagu is uint16_t. I don't remember
>> > what it gets promoted to offhand, but probably not uint32_t.
>>
>> it gets promoted to 'int'
>
> So use the correct conversion specifier for 'int'.

ok so a normal %X should do
-- 
Vittorio
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] cfhd: Use proper ISO C printf conversion specifiers

[Diego Biurrun]

On Tue, Apr 04, 2017 at 05:47:35PM +0200, Vittorio Giovara wrote:
> On Tue, Apr 4, 2017 at 5:10 PM, Diego Biurrun  wrote:
> > On Tue, Apr 04, 2017 at 04:39:23PM +0200, Vittorio Giovara wrote:
> >> libavcodec/cfhd.c:258:16: warning: format specifies
> >>   type 'unsigned short' but the argument has type 'int' [-Wformat]
> >> --- a/libavcodec/cfhd.c
> >> +++ b/libavcodec/cfhd.c
> >> @@ -254,11 +254,11 @@ static int parse_tag(CFHDContext *s, GetByteContext 
> >> *gb,
> >>
> >>  if (abs_tag8 >= 0x60 && abs_tag8 <= 0x6F) {
> >> -av_log(s->avctx, AV_LOG_DEBUG, "large len %"PRIX16"\n",
> >> +av_log(s->avctx, AV_LOG_DEBUG, "large len %"PRIX32"\n",
> >> ((tagu & 0xFF) << 16) | data);
> >
> > The warning does not match your change. tagu is uint16_t. I don't remember
> > what it gets promoted to offhand, but probably not uint32_t.
> 
> it gets promoted to 'int'

So use the correct conversion specifier for 'int'.

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] cfhd: Use proper ISO C printf conversion specifiers

[Luca Barbato]

On 04/04/2017 17:47, Vittorio Giovara wrote:
> On Tue, Apr 4, 2017 at 5:10 PM, Diego Biurrun  wrote:
>> On Tue, Apr 04, 2017 at 04:39:23PM +0200, Vittorio Giovara wrote:
>>> libavcodec/cfhd.c:258:16: warning: format specifies
>>>   type 'unsigned short' but the argument has type 'int' [-Wformat]
>>> --- a/libavcodec/cfhd.c
>>> +++ b/libavcodec/cfhd.c
>>> @@ -254,11 +254,11 @@ static int parse_tag(CFHDContext *s, GetByteContext 
>>> *gb,
>>>
>>>  if (abs_tag8 >= 0x60 && abs_tag8 <= 0x6F) {
>>> -av_log(s->avctx, AV_LOG_DEBUG, "large len %"PRIX16"\n",
>>> +av_log(s->avctx, AV_LOG_DEBUG, "large len %"PRIX32"\n",
>>> ((tagu & 0xFF) << 16) | data);
>>
>> The warning does not match your change. tagu is uint16_t. I don't remember
>> what it gets promoted to offhand, but probably not uint32_t.
> 
> it gets promoted to 'int'
> 

so a normal %d is enough
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] cfhd: Use proper ISO C printf conversion specifiers

[Vittorio Giovara]

On Tue, Apr 4, 2017 at 5:10 PM, Diego Biurrun  wrote:
> On Tue, Apr 04, 2017 at 04:39:23PM +0200, Vittorio Giovara wrote:
>> libavcodec/cfhd.c:258:16: warning: format specifies
>>   type 'unsigned short' but the argument has type 'int' [-Wformat]
>> --- a/libavcodec/cfhd.c
>> +++ b/libavcodec/cfhd.c
>> @@ -254,11 +254,11 @@ static int parse_tag(CFHDContext *s, GetByteContext 
>> *gb,
>>
>>  if (abs_tag8 >= 0x60 && abs_tag8 <= 0x6F) {
>> -av_log(s->avctx, AV_LOG_DEBUG, "large len %"PRIX16"\n",
>> +av_log(s->avctx, AV_LOG_DEBUG, "large len %"PRIX32"\n",
>> ((tagu & 0xFF) << 16) | data);
>
> The warning does not match your change. tagu is uint16_t. I don't remember
> what it gets promoted to offhand, but probably not uint32_t.

it gets promoted to 'int'
-- 
Vittorio
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] cfhd: Use proper ISO C printf conversion specifiers

[Diego Biurrun]

On Tue, Apr 04, 2017 at 04:39:23PM +0200, Vittorio Giovara wrote:
> libavcodec/cfhd.c:258:16: warning: format specifies
>   type 'unsigned short' but the argument has type 'int' [-Wformat]
> --- a/libavcodec/cfhd.c
> +++ b/libavcodec/cfhd.c
> @@ -254,11 +254,11 @@ static int parse_tag(CFHDContext *s, GetByteContext *gb,
>  
>  if (abs_tag8 >= 0x60 && abs_tag8 <= 0x6F) {
> -av_log(s->avctx, AV_LOG_DEBUG, "large len %"PRIX16"\n",
> +av_log(s->avctx, AV_LOG_DEBUG, "large len %"PRIX32"\n",
> ((tagu & 0xFF) << 16) | data);

The warning does not match your change. tagu is uint16_t. I don't remember
what it gets promoted to offhand, but probably not uint32_t.

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel