450196 - Don't use -Wconversion with GCC

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Zack Weinberg (:zwol)]

Depending on which version you have, GCC's -Wconversion option is either completely useless for C++ (4.2 and earlier, where it told you about bugs that might have been introduced while converting a code-base from *pre-standard C* to something more modern), or enables a set of warnings that have tons of false positives on the Mozilla code base (4.3 and later, where it complains about things like assigning a PRInt32 into a PRInt8 variable, whether or not the actual range of values that the wider variable takes is at risk of truncation).

Thus I recommend just dropping it altogether.

Comment 1

[Benjamin Smedberg]

This was discussed and originally rejected in bug 409384. I am still in favor and dbaron is opposed.

Comment 2

[Zack Weinberg (:zwol)]

It doesn't look to me like the -Wconversion issue, specifically, was addressed in all that much detail in that bug.  I'm going to paste in the GCC manual's description of that option.  In versions up through 4.2, it read:

    Warn if a prototype causes a type conversion that is different from 
    what would happen to the same argument in the absence of a prototype. 
    This includes conversions of fixed point to floating and vice versa,
    and conversions changing the width or signedness of a fixed point 
    argument except when the same as the default promotion.

    Also, warn if a negative integer constant expression is implicitly
    converted to an unsigned type. For example, warn about the assignment 
    x = -1 if x is unsigned. But do not warn about explicit casts like
    (unsigned) -1.

The first paragraph describes a warning that is *completely useless* for C++ or indeed for modern C, because if everything is prototyped, nobody expects anything else.  It'll complain for things like passing a value of type |float| to a function that's prototyped to take |float| arguments!  The second paragraph describes a warning that might conceivably catch real bugs, but in practice will be so swamped by the junk warnings from the first paragraph that it, too, is useless.

Recognizing this uselessness, the GCC team changed the effect of the option substantially in version 4.3.  The new documentation reads

    Warn for implicit conversions that may alter a value. This includes
    conversions between real and integer, like abs (x) when x is double;
    conversions between signed and unsigned, like unsigned ui = -1; and
    conversions to smaller types, like sqrtf (M_PI). Do not warn for
    explicit casts like abs ((int) x) and ui = (unsigned) -1, or if the
    value is not changed by the conversion like in abs (2.0). Warnings 
    about conversions between signed and unsigned integers can be disabled
    by using -Wno-sign-conversion.

    For C++, also warn for conversions between NULL and non-pointer types;
    confusing overload resolution for user-defined conversions; and
    conversions that will never use a type conversion operator: conversions
    to void, the same type, a base class or a reference to them. Warnings
    about conversions between signed and unsigned integers are disabled
    by default in C++ unless -Wsign-conversion is explicitly enabled. 

This sure sounds like it would be a useful warning - I can see where dbaron is coming from - but in practice it generates so much noise that it's no help at all.  I'm in the middle of a test run right now but later I'll provide some examples.

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I guess I'm ok with turning it off.

Comment 4

[Zack Weinberg (:zwol)]

I should say that I'd be happy to help with an effort to go through the code and make it clean for gcc4.3's -Wconversion so we can turn it back on -- but right now there's so much noise in some files it's hard to find the actual error messages coming from changes I made. :-/

Comment 5

[Benjamin Smedberg]

Pushed a fix to mozilla-central, revision 81039c7842be