symisc / PH7

An Embedded Implementation of PHP (C Library)
http://ph7.symisc.net
Other
494 stars 68 forks source link

Warnings while comparing signed and unsigned long integers #15

Closed sixman9 closed 8 years ago

sixman9 commented 8 years ago

I'm on OSX 10.11.6 (El Capitan), using GCC (LLVM/clang-703.0.31) and during compilation, I'm seeing the following, relatively harmless, integer comparison warnings:

2 warnings generated.
ph7.c:20546:65: warning: comparison of integers of different signs: 'ph7_int64' (aka 'long long') and 'unsigned long' [-Wsign-compare]
            n = pStream->xRead(pDev->pHandle,zBuf,(nMaxLen > 0 && nMaxLen < sizeof(zBuf)) ? nMaxLen : sizeof(zBuf));
                                                                  ~~~~~~~ ^ ~~~~~~~~~~~~
ph7.c:21422:29: warning: comparison of integers of different signs: 'ph7_int64' (aka 'long long') and 'unsigned long' [-Wsign-compare]
                    (nMaxlen > 0 && (nMaxlen < sizeof(zBuf))) ? nMaxlen : sizeof(zBuf));
                                     ~~~~~~~ ^ ~~~~~~~~~~~~

Could the warnings be resolved by casting the 'nMaxlen' variable, in the '(nMaxlen < sizeof(zBuf))' comparison to be 'unsigned'? I'm assuming the sign will make no difference, given the initial positive check (i.e. 'nMaxlen > 0').

In this post by Jamie Bullock@stackoverflow, he offers a simple example of how to better handle this type comparison mismatch:

Actually, I don't think turning off the compiler warning is the right solution, since comparing an int and an unsigned long introduces a subtle bug.

For example:

unsigned int a = UINT_MAX; // 0xFFFFFFFFU == 4,294,967,295 
signed int b = a; // 0xFFFFFFFF == -1

for (int i = 0; i < b; ++i)
{
    // the loop will have zero iterations because i < b is always false!
}

Basically if you simply cast away (implicitly or explicitly) an unsigned int to an int your code will behave incorrectly if the value of your unsigned int is greater than INT_MAX.

The correct solution is to cast the signed int to unsigned int and to also compare the signed int to zero, covering the case where it is negative:

unsigned int a = UINT_MAX; // 0xFFFFFFFFU == 4,294,967,295 

for (int i = 0; i < 0 || (unsigned)i < a; ++i)
{
    // The loop will have UINT_MAX iterations
}

I do realise I could simply suppress or ignore the warnings.

symisc commented 8 years ago

Pretty hramless, but very interesting. Thank you