797117 - 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]

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Daniel Holbert [:dholbert]]

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)

Comment 1

[Daniel Holbert [:dholbert]]

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

Comment 2

[(no longer active)]

So should we just static_cast our way out of this?

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix

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.

Comment 4

[(no longer active)]

Comment on attachment 667215 [details] [diff] [review]
fix

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

Sigh...

Comment 5

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e474cc38930b

Comment 6

[Jeff Walden [:Waldo]]

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.

Comment 7

[Jeff Walden [:Waldo]]

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.  :-)

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e474cc38930b