502301 - fix C++0x narrowing conversion inside {} compilation errors

Status: RESOLVED FIXED

(Core :: General, defect)

Description

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

Attachment: patch

According to gcc 4.4, at least, C++0x makes what it defines as "narrowing conversions" inside of {} into errors.  (I was using gcc 4.4 with -std=gnu++0x for bug 502298.)

The attached patch fixes these errors in the code I hit while compiling mozilla-central Firefox.

Comment 1

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

Comment on attachment 386782 [details] [diff] [review]
patch

Do you think we should land this?  I'm not sure what the standard actually says here; it seems a little odd for it to become stricter.

Comment 2

[Benjamin Smedberg]

Comment on attachment 386782 [details] [diff] [review]
patch

That does seem odd, especially in the case of literals which are not intrinsically too large. Let me ask around.

Comment 3

[Benjamin Smedberg]

Comment on attachment 386782 [details] [diff] [review]
patch

I think this makes the code less readable and we should only do it as a last resort.

Comment 4

[Mike Hommey [:glandium]]

From a draft:A narrowing conversion is an implicit conversion
(...)
- from long double to double or float, or from double to float (...)
(...)
- from an integer type or unscoped enumeration type to an integer type that cannot represent all the values of the original type, except where the source is a constant expression and the actual value after conversion will fit into the target type and will produce the original value when converted back to the original type.

If I take the changes from dbaron's patch in order:
- conversion of an int value > 127 to a signed char.
- conversion of floats to doubles.
- conversion of a int (PRBool) to an 8-bit int type (PRPackedBool).
- conversion of long unsigned ints (which can be 64 bits) containing values that are apparently supposed to be signed 32 bits values to a *signed* int (which is always 32 bits)
- conversion of a size_t (which can be 64 bits) to a 32 bits unsigned int
- conversion of an int value > 127 to a signed char.

Except the float to double conversions, none of these seem to be gcc being too pedantic.

Maybe some of these could be fixed by changing the types of the arrays, but I don't think there is much to expect from gcc's end.

Comment 5

[Mike Hommey [:glandium]]

FWIW, some more show up on powerpc and s390:
nsBinHexDecoder.cpp:119: error: narrowing conversion of '-0x00000000000000001' from 'int' to 'char' inside { }

Comment 6

[Mike Hommey [:glandium]]

(In reply to comment #5)
> FWIW, some more show up on powerpc and s390:
> nsBinHexDecoder.cpp:119: error: narrowing conversion of '-0x00000000000000001'
> from 'int' to 'char' inside { }

That should also appear on arm, and is due to char being signed on these architectures, contrary to other architectures where it is unsigned.

Comment 7

[Mike Hommey [:glandium]]

> That should also appear on arm, and is due to char being signed on these
> architectures, contrary to other architectures where it is unsigned.

Erm, replace unsigned with signed and vice versa.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Additional patch

Patch for the signed char issue in nsBinHexDecoder.cpp.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 386782 [details] [diff] [review]
patch

Re-requesting review per comment 4. The only change I'd personally do to this patch is on the xpcom/tests part, only changing the type instead of the values, but that really doesn't matter much.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 438743 [details] [diff] [review]
Additional patch

Actually, this patch is required independently of this bug, because there *is* a bug when char is unsigned: the decode table is then full of 255 instead of -1, and the test whether a given value in the table is equal to (PRUint32) -1 is always false.

Comment 11

[Benjamin Smedberg]

Comment on attachment 386782 [details] [diff] [review]
patch

ok. I think I understand a little better: for things like `const char expected = 0xEF;`, the error is only because 0xEF doesn't fit in a signed char. So `const char expected = 0xF;` would be ok?

Is there some flag we can add to GCC compile lines so that these types of conversions issue warnings/errors in C++ mode, not just C++0x mode?

Comment 12

[Mike Hommey [:glandium]]

-Wsign-compare should take care of the char signedness problem, but I guess it won't display a warning unless char is actually unsigned. I haven't found another option that would warn for the c++0x errors fixed in this bug, but I'm not sure I was looking at a recent manual page.

Comment 13

[Mike Hommey [:glandium]]

The TestEncoding.cpp part was fixed in bug 552805. Apparently, someone got a warning for that one.

Comment 14

[Mike Hommey [:glandium]]

http://hg.mozilla.org/mozilla-central/rev/1f6a2d30ed6e
http://hg.mozilla.org/mozilla-central/rev/6539fa13490b