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

[Michael Niedermayer]

On Wed, May 16, 2012 at 01:45:15PM +0200, Reimar Döffinger wrote:
> 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.

iam happy with anything but i would like to see the warnings fixed
or worked around because they hide real warnings in a forrest of crap
on some compiler.

so iam going to apply whatever best and most agreed upon exists in
a few days or a week. But if you apply/push some solution before iam
even happier as its one patch less for me to keep track of (and i
could forget ...)

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 3
"Rare item" - "Common item with rare defect or maybe just a lie"
"Professional" - "'Toy' made in china, not functional except as doorstop"
"Experts will know" - "The seller hopes you are not an expert"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120516/13bc6d60/attachment.asc>