Fixing signed/unsigned warnings?

I'm trying to compile some very bad code that gives me dozens of warnings like:

Code:

---

warning C4018: '>=' : signed/unsigned mismatch

---

for code such as:

Code:

---

if ( wcslen(comma) >= bufLen )

---

and I'm wondering what the best way to get rid of these warnings?
I can't make bufLen unsigned because it would break the API that's already used by many different products.

I can see 3 choices:
- Use a #pragma to disable the warning, but that's just sloppy and compiler specific.
- Cast the unsigned int to an int.
- Cast the int to an unsigned int.

Do I just flop a coin about which side to cast, or is one choice better than the other?

My guess is, if these are both supposed to represent lengths, then you should cast to unsigned int. Do you know how bufLen gets a negative value? Is it set to a negative sentinel, like -1, that you should watch for, or is it just so large that it's stored negative?

Well, it depends on the possible values of the two variables. Example: if the unsigned is actually a size_t and the signed is a counter, then the obvious choice is to cast unsigned to the counter (int) since there's almost no risk (if any) of losing any data.

EDIT: After looking at it again (and seeing tabstop's reply) I realized that they are both lengths so, I guess you should just cast unsigned to the int.

Thanks,
I guess if a negative buf length is passed in that's a whole other bug, so I just casted it to unsigned int.

Now, what about variables that aren't lengths, and are completely undocumented (just like most of this code)?

BTW, what's the default conversion that the compiler makes if you just ignore the warning? Does it convert them both to int, or both to unsigned ints?

I think the integer promotions are the same in C++ as in C. The unsigned versions are always of higher rank than the signed versions, so I would guess things would promote to unsigned. (I'm not in the same place as my standard, and it's a C standard, not C++ anyway.)

Why is bufLength signed in the first place?

Quote:

---

Originally Posted by CornedBee

Why is bufLength signed in the first place?

---

Because this code sucks. It also has port numbers as DWORDs instead of unsigned shorts, and it checks if ( some_unsigned_number < 0 )... :p

Quote:

---

Originally Posted by tabstop

I think the integer promotions are the same in C++ as in C. The unsigned versions are always of higher rank than the signed versions, so I would guess things would promote to unsigned. (I'm not in the same place as my standard, and it's a C standard, not C++ anyway.)

---

Yes, that's how it is in C++ as well as in C. They wouldn't have changed such a basic rule.

If the code always uses signed variables like this, you could use

Code:

---

#define wcslen(str) ((signed long)wcslen(str))

---

or something. An ugly hack in a sea of them, no doubt. I'd probably just ignore the warnings if I was just trying to compile the code.

Quote:

---

Originally Posted by dwks

Yes, that's how it is in C++ as well as in C. They wouldn't have changed such a basic rule.

If the code always uses signed variables like this, you could use

Code:

---

#define wcslen(str) ((signed long)wcslen(str))

---

or something. An ugly hack in a sea of them, no doubt. I'd probably just ignore the warnings if I was just trying to compile the code.

---

I hate ignoring warnings, because eventually you'll have so many of them that you'll miss the important ones. I already found a few serious bugs by fixing the warnings; but I don't want to start pulling at too many loose threads since the whole thing might fall apart. :p

Why don't you make some tests to run after each build or something?

Unit testing or something. I don't know how to do it, just that it might be useful. :p

Quote:

---

Originally Posted by dwks

Yes, that's how it is in C++ as well as in C. They wouldn't have changed such a basic rule.

---

Yes, but I just had weird visions of

Code:

---

bool operator < (const int& left, const unsigned int& right)

---

running through my mind. :) I need to get out more.

Perhaps juse make bufLen unsigned anyway, and use a cast in the calls to the API.
That makes it obvious where the incorrect type is being used - the API itself.

Quote:

---

Originally Posted by robwhit

Why don't you make some tests to run after each build or something?

Unit testing or something. I don't know how to do it, just that it might be useful. :p

---

It's not my code. I'm just fixing a bug in it since the person that normally takes care of it is busy with another project. Trust me, if it was my code it would be running like a well oiled machine. ;)

Quote:

---

Originally Posted by iMalc

Perhaps juse make bufLen unsigned anyway, and use a cast in the calls to the API.
That makes it obvious where the incorrect type is being used - the API itself.

---

I'm not very familiar with the code and I certainly don't know what other code calls this function, so I'm just leaving it as is. I'm just casting away the warning and adding a big ugly comment about the warning I was getting...

I would also add an assert(buflen >= 0) (or if-statement with a printout, or similar) just above the line with the cast [if there are several casts on the same (unchanged) variable, you obviously only need one assert]. That way, _IF_ the bufLen is ever negative, you have a chance to catch it.

--
Mats

Quote:

---

Originally Posted by matsp

I would also add an assert(buflen >= 0) (or if-statement with a printout, or similar) just above the line with the cast [if there are several casts on the same (unchanged) variable, you obviously only need one assert]. That way, _IF_ the bufLen is ever negative, you have a chance to catch it.

--
Mats

---

Yeah, I guess since assert() only aborts in debug builds it should be safe to add it. I'll probably do that. I don't know if anyone on that team ever tests using debug builds though. :p