[FFmpeg-devel] [PATCH] Fix warnings about int64 toint32conversion

[Don Moir]

>>> Don Moir <donmoir <at> comcast.net> writes:
>>>
>>>> and you don't want to have to disable warning C4244 in general.
>>>
>>> (Why?)
>>> I assume it is possible to disable (and re-enable) the warning
>>> with a pragma directive, please find out how it has to look and
>>> send a patch, it will probably not affect sane compilers.
>>>
>>
>> Thats what I do when I include ffmpeg headers (prefer not to), but why 
>> pass this down to everyone else that might run into this ?
>
> He was asking for a pragma, and my guess is that the idea would be to add 
> the pragma to the header which in the long term at least is likely to mean 
> less clutter and is more self-explantory.
> I admit I am not sure how well it would work out in reality though.

I think the pragma in ffmpeg would be more clutter then the cast but I have 
a ffmpeg.h file that does whatever for my needs. For the include part it 
does this:

extern "C"
{
    #pragma warning(disable:4244)
    #include <libavcodec/avcodec.h>
    #include <libavformat/avformat.h>
    #include <libswscale/swscale.h>
    #include <libswresample/swresample.h>
    #include <libavutil/opt.h>
    #pragma warning(default:4244)
}

The #pragma warning number is going to be compiler specific but this does 
the trick for MS compiler. I would prefer not to use the pragma so I will 
know if I get a warning but it can be so annoying that I do it for ffmpeg.

> First: my main point was that there are two warnings quoted and more than
> two casts added, so I wanted explicit confirmation that each and every one 
> is really needed.

There are 3 inline functions effected and 5 total warnings. I modified these 
just a bit to avoid email wrapping and to be more clear.

av_clipl_int32_c and av_popcount64_c are in libautils\common.h and av_cmp_q 
in libautil\rational.h

The following case produces 2 warnings because even though you told the 
compiler the return type, it warns of the possible conversion loss. That is, 
from int64_t to int32_t.

static av_always_inline av_const int32_t av_clipl_int32_c(int64_t a)
{
    if ((a+0x80000000u) & ~UINT64_C(0xFFFFFFFF))
        return (a>>63) ^ 0x7FFFFFFF;
    else
        return a;
}

In the above case to avoid the warnings it should be modified to:

static av_always_inline av_const int32_t av_clipl_int32_c(int64_t a)
{
    if ((a+0x80000000u) & ~UINT64_C(0xFFFFFFFF))
        return (int32_t)((a>>63) ^ 0x7FFFFFFF);
    else
        return (int32_t)a;
}

Now thats just a little more clutter but it prevents the warnings from 
propagating to everyone else. If it was C++ code, I would use the cleaner 
cast style of:

return int32_t((a>>63) ^ 0x7FFFFFFF); and return int32_t(a);

The other 2 functions effected are:

2 warning for the 2 av_popcount calls in av_popcount64_c

static av_always_inline av_const int av_popcount64_c(uint64_t x)
{
-    return av_popcount(x) + av_popcount(x >> 32);
+   return av_popcount((uint32_t)x) + av_popcount((uint32_t)(x >> 32));
}

static inline int av_cmp_q(AVRational a, AVRational b){
    const int64_t tmp= a.num * (int64_t)b.den - b.num * (int64_t)a.den;
-    if(tmp) return ((tmp ^ a.den ^ b.den)>>63)|1;
+    if(tmp) return (int)(((tmp ^ a.den ^ b.den)>>63)|1);
    else if(b.den && a.den) return 0;
    else if(a.num && b.num) return (a.num>>31) - (b.num>>31);
    else                    return INT_MIN;
}

> Second: If really all these casts are needed the I believe a compiler bug 
> report
> is in order, since there is really no excuse for printing a warning for 
> some of those.
> Concerning clutter: I generally consider any superfluous cast a rather bad 
> kind of
> clutter, in C the casts are hard enough to grep for, adding a few with no 
> purpose
> does not make things better.

I am glad you are picky about such things truely and I wish more were picky 
about there own code. This is more of a practical matter and easy to solve 
in a couple ways but these are the only changes that need to be made and all 
the warnings will go away for the primary headers.