Closed Bug 502301 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: dbaron, Assigned: dbaron)

Details

Attachments

(2 files)

Attached patch patchSplinter Review
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 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.
Attachment #386782 - Flags: review?(benjamin)
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 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.
Attachment #386782 - Flags: review?(benjamin) → review-
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.
FWIW, some more show up on powerpc and s390:
nsBinHexDecoder.cpp:119: error: narrowing conversion of '-0x00000000000000001' from 'int' to 'char' inside { }
(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.
> 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.
Attached patch Additional patchSplinter Review
Patch for the signed char issue in nsBinHexDecoder.cpp.
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.
Attachment #386782 - Flags: review- → review?(benjamin)
Attachment #438743 - Flags: review?(benjamin)
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 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?
Attachment #386782 - Flags: review?(benjamin) → review+
Attachment #438743 - Flags: review?(benjamin) → review+
-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.
The TestEncoding.cpp part was fixed in bug 552805. Apparently, someone got a warning for that one.
Target Milestone: --- → mozilla1.9.3a5
You need to log in before you can comment on or make changes to this bug.