Fix final sign comparison warnings #1180

pull jgarzik wants to merge 4 commits into bitcoin:master from jgarzik:sign-compare changing 4 files +11 −11

jgarzik commented at 9:59 PM on May 1, 2012: contributor

These commits should clean up the last of the signed comparison warnings. I'd say we can turn on -Wall now.
CDiskTxPos, CInPoint, COutPoint: cast null value (-1) to unsigned int
to eliminate signed/unsigned comparison warnings
24de922636

OpenBlockFile(): cast to eliminate signed/unsigned comparison warning

nFile's null value is -1.  Cast that to unsigned int, to avoid warning.

Additionally, avoid nFile==0 because the first valid value is 1.

10ab9c2f42

ThreadSocketHandler2(): cast to avoid signed/unsigned warning 061a001590
EvalScript(): cast to avoid signed/unsigned warning 024fa1cb44
gavinandresen commented at 3:59 PM on May 2, 2012: contributor

ACK
sipa commented at 1:16 AM on May 3, 2012: member

Survives my tests. ACK.
in src/main.cpp:None in 024fa1cb44
```
1835 | @@ -1836,7 +1836,7 @@ bool CheckDiskSpace(uint64 nAdditionalBytes)
1836 |  
1837 |  FILE* OpenBlockFile(unsigned int nFile, unsigned int nBlockPos, const char* pszMode)
1838 |  {
1839 | -    if (nFile == -1)
1840 | +    if ((nFile < 1) || (nFile == (unsigned int) -1))
```
laanwj commented at 5:56 AM on May 3, 2012:

We could define a constant for (unsigned int) -1, as it's a magic marker value we use in many places.

Diapolo commented at 4:45 PM on May 4, 2012:

That would be something like that loop typedef, which we considered "bad", right?
in src/net.cpp:None in 024fa1cb44
```
 620 | @@ -621,7 +621,7 @@ void ThreadSocketHandler2(void* parg)
 621 |          if (nSelect == SOCKET_ERROR)
 622 |          {
 623 |              int nErr = WSAGetLastError();
 624 | -            if (hSocketMax > -1)
 625 | +            if (hSocketMax > (SOCKET) -1)
```
laanwj commented at 5:59 AM on May 3, 2012:

Is this code path ever followed? I mean, will an unsigned integer ever be larger than (unsigned) -1?

sipa commented at 12:51 PM on May 3, 2012:

Quite sure it won't; maybe on other platforms?

laanwj commented at 1:24 PM on May 4, 2012:

Fixing signed/unsigned comparison warnings is a good opportunity to see if there's illogical/insecure stuff. But in this case, I'm not sure what was the intended behavior.

Diapolo commented at 4:41 PM on May 4, 2012:

hSocketMax is 0 or set by some magic, happening in the FD_SET macro if I read correctly.

Diapolo commented at 4:47 PM on May 4, 2012:

It was introduced by this commit from @TheBlueMatt: https://github.com/bitcoin/bitcoin/commit/c6710c7a70658536ab0217dff18a45622ea08680

sipa commented at 4:48 PM on May 4, 2012:

socket() returns -1 in case of error, and the constant INVALID_SOCKET reflects this. Not sure if it's related.

Diapolo commented at 4:50 PM on May 4, 2012:

On Windows (winsock2.h) SOCKET_ERROR has the value -1. So should it perhaps read (hSocketMax != INVALID_SOCKET && hSocketMax != SOCKET_ERROR)?

laanwj commented at 5:17 PM on May 4, 2012:

If INVALID_SOCKET and SOCKET_ERROR are both -1, there's no need to compare against both. I agree replacing > with != is sensible, though.

Diapolo commented at 5:20 PM on May 4, 2012:

That's why I used both:

#define INVALID_SOCKET (SOCKET)(~0) #define SOCKET_ERROR (-1)

laanwj commented at 5:29 PM on May 4, 2012:

Both evaluate to binary all-ones and are equivalent on all architectures with two's complement notation for signed numbers (ie, at least all that windows supports.. and certainly that bitcoin supports).

Diapolo commented at 5:48 PM on May 4, 2012:

Didn't know that, but I think the names are more speaking than a simple -1 :), look how we guessed what that -1 means here. Would not have happend if we used the error codes.
jgarzik commented at 8:18 PM on May 8, 2012: contributor

It is definitely a valid point that " if (hSocketMax > (SOCKET) -1)" is an imperfect test. Probably should use != etc.

However, that can be addressed in a separate pull request. This pull req does not change the test itself...
Diapolo commented at 8:30 PM on May 8, 2012: none

@jgarzik Will you do that mentioned pull above or shall I do it tomorrow?
jgarzik commented at 8:38 PM on May 8, 2012: contributor

feel free to do so...
jgarzik referenced this in commit a2ea797593 on May 8, 2012
jgarzik merged this on May 8, 2012
jgarzik closed this on May 8, 2012
coblee referenced this in commit 0c2a2bfde5 on Jul 17, 2012
suprnurd referenced this in commit 80444ea8e4 on Dec 5, 2017
lateminer referenced this in commit 6374175980 on Jan 22, 2019
lateminer referenced this in commit 4f4da456de on Dec 25, 2019
Bushstar referenced this in commit 2aaa1282de on Oct 21, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Milestone
0.7.0

Linked (view graph)

#1225 fix an incorrect if-clause in net.cpp