514107 - add -Wtype-limits to gcc C++ compilation warnings

Status: RESOLVED DUPLICATE of bug 1018288

(Firefox Build System :: General, defect)

Description

[Karl Tomlinson (:karlt)]

Bug 503784, bug 513789, and bug 513807 are situations where the code was not
doing what the author intended, but could have been detected by gcc's
-Wtype-limits warnings.

These have been enabled through -W (= -Wextra) for C code since 1999
but it's not clear why it wasn't enabled for C++.

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla&command=DIFF_FRAMESET&file=configure.in&rev2=1.522&rev1=1.521
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/configure.in&rev=1.522

Prior to that -W was removed from both C and C++ options "to get rid of
"unused parameter" noise"

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=configure.in&root=/cvsroot&subdir=mozilla&command=DIFF_FRAMESET&rev1=1.303&rev2=1.304
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/configure.in&rev=1.304

This is no longer a good reason as -Wno-unused-parameter is available at least
as far back as gcc-3.3.6.

With gcc 4.3, type-limits warnings can be enabled without turning on any other
-Wextra warnings, if that is preferred.  (Some -Wextra warnings still cannot be controlled individually.)

http://gcc.gnu.org/onlinedocs/gcc-4.3.4/gcc/Warning-Options.html

Comment 1

[Karl Tomlinson (:karlt)]

There are several places in current code that assert parameter ranges that
would show this warning due to asserting unsigned > 0, but these would be
trivial to suppress (but removing the unnecessary assertion).

The one place that I've seen where suppression would be non-trivial is bug
458722.  The warning cannot be suppressed by using a cast, but can be
suppressed by using a non-const intermediate variable:

#include <stddef.h>
int f1(unsigned short capacity) {
  if (size_t(capacity) > ~(size_t)0 / sizeof(short)) return 2;
  return 3;
}
int f2(unsigned short capacity) {
  const size_t max = ~(size_t)0 / sizeof(short);
  if (capacity > max) return 2;
  return 3;
}
int f3(unsigned short capacity) {
  size_t max = ~(size_t)0 / sizeof(short);
  if (capacity > max) return 2;
  return 3;
}

type-limits.cpp: In function 'int f1(short unsigned int)':
type-limits.cpp:3: warning: comparison is always false due to limited range of data type
type-limits.cpp: In function 'int f2(short unsigned int)':
type-limits.cpp:8: warning: comparison is always false due to limited range of data type

This looks like trying to defeat the optimizer, but all three functions
do generate the same code with -O1.

Using macros like POPULATE doesn't really look like elegant code to me (and perhaps the warning has kind of detected that), but it does indicate that sometimes suppressing false positive warnings may be inconvenient.

However, these situations seem to be rare and the warnings do flag real bugs.

Comment 2

[Karl Tomlinson (:karlt)]

It would be nice to enable type-limits warnings for versions of gcc prior to
4.3, particularly so that people using Apple's gcc get the warnings.  This
could be done by using "-W -Wno-unused-parameter
-Wno-missing-field-initializers".  (-Wmissing-field-initializers is available
in gcc-4.0.4 but not 3.4.6.)

The main reason not to do that seems to be the following warning (though maybe
the pain would be worth it?):

"warning: enumeral and non-enumeral type in conditional expression"

The documentation for this warning says "The option -Wextra also prints
warning messages [when] An enumerator and a non-enumerator both appear in a
conditional expression."

However, I think the warning is emitted only when the integral promotion of
the underlying type of the enumerator differs from the non-enumerator
(integral) type.  http://gcc.gnu.org/ml/gcc-patches/1999-04/msg00596.html

It's not clear to me what good this warning would be.
And it seems to be unclear to others also:
http://gcc.gnu.org/ml/gcc-bugs/2000-07/msg00165.html

Trying to suppress by making integral constants the same type as the
underlying type of the enumerator is difficult because the underlying type
depends on other values in the enumeration type and is implementation defined.

Suppressing these warnings would involve explicitly casting (almost) all
enumerators (in conditional expressions with integral types) to the
integral type.

I can see a possible error here if the integral type is signed and an
enumerator with unexpected unsigned underlying type causes promotion of the
integral type to unsigned (possibly changing the value).  In this situation
casting the enumerator to signed seems sensible, but I don't see why it should
ever be necessary to cast enumerators to unsigned.

-Wsign-conversion (in gcc-4.3) seems a better warning for this:

  "Warn for implicit conversions that may change the sign of an integer value,
   like assigning a signed integer expression to an unsigned integer
   variable."

int f1(int i) {
  enum { E = 10, MAX = 0x80000000U };
  return i ? E : i;
}
unsigned int f2(unsigned int i) {
  enum { E = 10U };
  return i ? E : i;
}

: In function 'int f1(int)':
:3: warning: enumeral and non-enumeral type in conditional expression
:3: warning: conversion to 'unsigned int' from 'int' may change the sign of the result
:3: warning: conversion to 'int' from 'unsigned int' may change the sign of the result
: In function 'unsigned int f2(unsigned int)':
:7: warning: enumeral and non-enumeral type in conditional expression

The conversion warning in function f1 is good, but if we enable
-Wsign-conversion, we'll have ~9670 other warnings.

The enumeral type warning in f2 seems unhelpful.  (Note that the type of the
initializer 10U does not affect the underlying type of the enumeration, which
gcc chooses to be signed.)

There are only a few of places where the enumeral type warning is showing up,
so it could be suppressed through casts if considered worthwhile.  Most of
these seem to be because const PRUint32 idl identifiers get converted to
anonymous enums (bug 239460), which usually have signed underlying type with
gcc.

Comment 3

[Chris Peterson [:cpeterson]]

Fixed by bug 1018288.