Closed Bug 797117 Opened 7 years ago Closed 7 years ago

GCC 4.7 build error, with "--enable-warnings-as-errors": nsSocketTransportService2.cpp:793:82: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]

Categories

(Core :: Networking, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

In a build with "ac_add_options --enable-warnings-as-errors" and GCC 4.7, I get this build error:
> netwerk/base/src/nsSocketTransportService2.cpp:793:82: error:
>   comparison between signed and unsigned integer expressions [-Werror=sign-compare]

...from this line:
> 793                 if (NS_UNLIKELY(pollInterval > (UINT16_MAX - s.mElapsedTime)))
https://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsSocketTransportService2.cpp?rev=c7df09766614#793

...which was last touched in this commit:
https://hg.mozilla.org/mozilla-central/rev/2c694d8bf7a5
Bug 791906: Replace NSPR integer limit constants with stdint ones; r=ehsan
 (Tentatively blaming that bug)
Summary: nsSocketTransportService2.cpp:793:82: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] → GCC 4.7 build error, with "--enable-warnings-as-error": nsSocketTransportService2.cpp:793:82: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
Summary: GCC 4.7 build error, with "--enable-warnings-as-error": nsSocketTransportService2.cpp:793:82: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] → GCC 4.7 build error, with "--enable-warnings-as-errors": nsSocketTransportService2.cpp:793:82: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
I bet this is related to this point that Isaac brought up in bug 797117 comment #5:
> * On my Linux system (and probably others), <stdint.h> does not explicitly
> set some of its UINTn_MAX [1] constants as unsigned -- this causes
> conflicting type errors
So should we just static_cast our way out of this?
Attached patch fixSplinter Review
I think so, yeah.

Note: the types of the values in the expression are:
 pollInterval:    uint32_t
 UINT16_MAX:      ??? (just a raw number, I think, so probably int32_t?)
 s.mElapsedTime:  uint16_t

If we just explicitly cast UINT16_MAX to uint32_t, then we should be fine.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #667215 - Flags: review?(ehsan)
Comment on attachment 667215 [details] [diff] [review]
fix

Review of attachment 667215 [details] [diff] [review]:
-----------------------------------------------------------------

Sigh...
Attachment #667215 - Flags: review?(ehsan) → review+
The exact-size limit macros, according to C99, have the type of the corresponding exact-size type "converted according to the integer promotions".  Which is gobbledygook to the effect that:

* the limit macro has the corresponding type if the type is int-sized or larger
* otherwise, the limit macro has type int if the corresponding type's values all fit in the int range, and type unsigned int if they all fit in the unsigned int range

Which in practice means UINT8_MAX will be int as all uint8_t fit in int (likewise for INT8_MAX), probably the same for the 16-bit macros, and for the 32-bit macros and larger they'll have the signedness of the corresponding type.

So then, adding the cast here looks right.  If I go by comment 3 you could have cast to uint16_t, but given UINT16_MAX is a known constant, the compiler should equally optimize the arithmetic either way.
I suppose, for completeness, I should note that -- I think -- the Windows stdint.h emulation doesn't match exactly exactly these rules, and UINT8_MAX has type uint8_t.  But I think that would only matter in really really really obscure edge cases (think overloading, or template parameter overloading) that we should probably just not do anyway.  :-)
https://hg.mozilla.org/mozilla-central/rev/e474cc38930b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.