[FFmpeg-devel] [PATCH] Fix warnings about int64toint32conversion

[Michael Bradshaw]

On Mon, May 21, 2012 at 11:16 AM, Don Moir <donmoir at comcast.net> wrote:
> ----- Original Message ----- From: "Michael Bradshaw"
> <mbradshaw at sorensonmedia.com>
>
> To: "FFmpeg development discussions and patches" <ffmpeg-devel at ffmpeg.org>
> Sent: Monday, May 21, 2012 12:56 PM
>
> Subject: Re: [FFmpeg-devel] [PATCH] Fix warnings about
> int64toint32conversion
>
>
>> On Wed, May 16, 2012 at 12:05 PM, Michael Bradshaw
>> <mbradshaw at sorensonmedia.com> wrote:
>>>
>>> Wow, this turned into a lively discussion.
>>>
>>> To answer some of the questions: these warnings are from MSVC 2008 and
>>> 2010 (haven't tested other versions). Yes, two of the casts are
>>> technically unnecessary (read below). I have tested this with gcc
>>> 4.2.1 with a random sampling of 1000000 integers, and the output is
>>> the same before and after the patch.
>>>
>>> I've attached a new version of the patch that removes the two
>>> unnecessary casts. I originally added them because a) I have a habit
>>> of being explicit when demoting data types and b) for consistency. I
>>> think you have a valid point Reimar that they probably shouldn't be
>>> included in this patch, though, and MSVC 2008 and 2010 don't complain
>>> if the two casts are removed.
>>>
>>> Let me know if this patch is better.
>>>
>>> Thanks,
>>>
>>> Michael
>>
>>
>> Ping.
>>
>> Reimar said he has no objections to this newer patch. Does anyone else?
>
>
> Was av_cmp_q in rational.h taken care of ? Don't remember seeing that in
> proposed patch.

It was not, no. I don't get any warnings from that function in VS 2008
or 2010, so I didn't modify it.

I just turned my warning level up to W4 and got several more warnings
in libavutil/common.h (but I still can't get av_cmp_q to give any
warnings; have you set some compiler flags or something?). Example of
where these warnings show up with the W4 flag:

static av_always_inline av_const uint8_t av_clip_uint8_c(int a)
{
    if (a&(~0xFF)) return (-a)>>31;
    else           return a; // WARNING HERE: "conversion from 'int'
to 'uint8_t', possible loss of data"
}

The rest of the warnings are similar, in functions av_clip_int8_c,
av_clip_uint16_c, and av_clip_int16_c. I know the if statement takes
care of overflow, and that these 4 new functions will not actually
overflow, but I still think it would be nice to explicitly fix the
warnings (by casting) rather than hide them. Reimar, I know you won't
like this, but do you think you'd object to changing the above
function to:

static av_always_inline av_const uint8_t av_clip_uint8_c(int a)
{
    if (a&(~0xFF)) return (-a)>>31;
+    else           return (uint8_t)a;
-    else           return a;
}

?

I'll work on a new patch when I get a couple minutes to spare on my
other dev partition. Don, you may need to submit your own patch for
changes to av_cmp_q because I can't reproduce the warning, and while
the fix is trivial, I'd like to be able to validate it in my own
testing if I were to submit it in a patch.

Thanks,

Michael