Re: [lmi] '-Wsign-promo' seems not to flag anything important

[Vadim Zeitlin]

On Thu, 21 Mar 2019 16:44:25 +0000 Greg Chicares <address@hidden> wrote:

GC> We'll soon upgrade gcc, so I figured I'd try some warning options
GC> that we haven't enabled yet.

 There are probably quite a few of them... Even if lmi enables many of
the warnings, I don't think it's close to the number of warnings disabled
by default:

        % g++-8 -Q --help=warning|fgrep '[disabled]'|wc -l
        182

Some of them are certainly non-applicable anyhow, but there must still be a
number of warnings that could be enabled but aren't, currently.

GC> With gcc-7.3 at least (I'll try these things with gcc-8.2 later, but
GC> for now lmi production uses 7.3), '-Wsign-promo' gives 414 lines of
GC> "error:" output, all of which are like this pair:
GC> 
GC> /opt/lmi/src/lmi/dbnames.cpp:53:44: error: passing 'e_database_key' chooses 
'int' over 'long unsigned int' [-Werror=sign-promo]
GC> /opt/lmi/src/lmi/dbnames.cpp:53:44: error:   in call to 
'std::basic_ostream<_CharT, _Traits>& std::basic_ostream<_CharT, 
_Traits>::operator<<(int) [with _CharT = char; _Traits = 
std::char_traits<char>]' [-Werror=sign-promo]
GC> 
GC> I.e., in each flagged case, I pass an enumerative type and am warned
GC> that it's treated as (signed) int...which is exactly how I'd expect
GC> it to be treated. IOW, even if the enumeration's underlying type is
GC> unsigned, to me that's just an artificial detail, as I think of 'int'
GC> as the One True non-floating arithmetic type. (I wouldn't have been
GC> shocked if gcc presupposed some other One True Type, but...see below.)

 It looks like gcc used to presuppose something different, as the
documentation of the option says:

        Warn when overload resolution chooses a promotion from unsigned or
        enumerated type to a signed type, over a conversion to an unsigned
        type of the same size. Previous versions of G++ tried to preserve
        unsignedness, but the standard mandates the current behavior. 

So it used to convert to unsigned for some reason, but now does the right
thing and the warning seems to exist solely to help people migrating from
old (ancient? I can't remember a gcc version which did this differently)
gcc versions. As such, it seems completely useless.

GC> I'm certainly not going to make changes like this:
GC> -    << "Unknown modal premium type " << premium_type << '.'
GC> +    << "Unknown modal premium type " << static_cast<int>(premium_type) << 
'.'

 No, this doesn't look useful. OTOH outputting enums like this always looks
suspicious to me as it seems to be almost invariably less useful than using
their string representation and I wouldn't mind changing the code to use
strings everywhere.

 I wish we could replace all the X-macros-based MCE machinery with
something more modern (it would still require some macros, but it should be
possible to make the implementation clearer) and switch to using scoped
enums instead of the C-style ones. Of course, then the code like above
wouldn't compile at all, but this, IMO, wouldn't be a bad thing, as it
would force us to use strings in the situations like above.

GC> AFAICS, it's best to leave this warning disabled, and maybe turn it on
GC> temporarily from time to time to see whether it finds anything important.

 I think we can perfectly well just leave it disabled.

 Regards,
VZ