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

[Reimar Döffinger]

On Wed, May 16, 2012 at 07:18:57AM -0400, Don Moir wrote:
>        return (a>>63) ^ 0x7FFFFFFF;

There is no loss possible here.
(int32_t)((a>>63) ^ 0x7FFFFFFF) == (a>>63) ^ 0x7FFFFFFF
always.
(the >> 63 means there is only one bit of information left,
the xor increases that again, but still the only possible values
are 2^32 - 1 and -(2^32) which are both exactly representable).
Thus I'd consider that a compiler bug we'd usually ask to be
reported upstream before considering a workaround.
If they decide to make such an annoying warning the default,
IMHO they need to spend the effort to make it get right at
least in the simpler cases like this.
I don't expect it to get the else part right (strictly speaking
no information loss is possible there either), that I admit
might be too difficult to figure out.

> 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);

I can't see what is cleaner about that, I'd consider it worse,
a misuse of constructors.
I'd strongly advise to use static_cast etc. with C++, that makes
it easy to grep for and thus at a later time verify that all
your casts are actually correct.
Though I admit this is off-topic.

> 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));

That is just plain ridiculous. I find it hard to believe the compiler
can't figure out that x>>32 is a 32 bit value.
On the other hand it also claims that
unsigned < (bool ? 1 : 2) is a comparison between signed and unsigned
*facepalm*. And yes I know why, it is still beyond stupid.

> -    if(tmp) return ((tmp ^ a.den ^ b.den)>>63)|1;
> +    if(tmp) return (int)(((tmp ^ a.den ^ b.den)>>63)|1);

This one is also trivial even for a compiler to see no information
loss is actually possible.
I don't object much to anything people agree on, but with only
2 out of those 5 warnings being reasonable I'd say this compiler
warning just isn't ready for prime time and never should have
been enabled by default.
So my expectation would be an open bug report on the compiler
as prerequisite to put workarounds in.
I have a slight dislike but no objections to the other 2 cases,
though comments or pragma use should still be considered.