VC6++ warning C4018 signed/unsigned mismatch

#1 their are even more if you look closer, a ton of type mitch matches. converting from float to int etc and screaming about possible data lose etc..

I have really never looked into them much, thought it would be nice if someone did.

#2 The ljpeg library is a free 3rd party library which we include without any changes. As long as it works we just leave it alone. If you crank up the VC6 warning level you get many more warnings in the Torque itself.

Much of the Torque library was originally built with an older version of CodeWarrior which did not produce these warnings. When the port to MSVC was made there wasn't time to fix all the warnings, so the warning level was just turned down. It's been that way ever since then :(

#3 I would be willing to go through the engine and try and fix as much of the warnings as possible. The type warnings are easy to fix since most of the time someone accidentally uses an int instead of an S32 or U32 or what have you. Other times, it requires a cast of some kind.

Since I build the engine using GCC2 I get a bunch of other warnings as well. It would be fun and challenging to see if all of them could get fixed but that might be a rather unrealistic goal, who knows :)

#4 I believe Chris Rioux (the patch I just integrated) is in the process of fixing all VC7 level 2 warnings, which I believe will include the U32 to S32 warnings. I'll keep you posted.

#5 *two year bump*

Out of date, I know, but the fancy new Google search capabilities on the site made it easy to find this one. ;-)

Was there any progress on this? I've just started using Torque, and find my usual way of working (Level 3 warnings and 'warnings as errors') just won't work. Since my custom code is mostly going to live in extra files, I'm sure I can apply the higher standard to just those files, but I thought I would check anyway...

#6 There are still tonnes of warnings throughout the engine... People gripe about it all the time, but none of the people who gripe ever take the time to fix it (to busy griping)...

I was using a different word choice, but decided to switch to gripe, as even mild language is offensive anymore.

#7 Very respectful choice of words. :P

If someone went through and fixed 'em, I'd be happy to merge in the fixes (assuming they really fix the underlying issues the warnings are about - I wouldn't be so thrilled about adding billions of typecasts just to get warnings to shut up...).

#8 Hmm, let me get some counts of the warnings at levels 1 and 2 so I can see the real scale of the task. I don't have a lot of spare time, but I might have enough.

#9 Okay, I can fix about 450 of the 800 or so relevant warning messages at Level 2 in an hour or so (there are lots and lots of constants with slack types). Those should be extremely easy to merge as well, as they're simply adding an 'f' or '.0f' suffix onto numeric constants.

In any previous refactoring work I've done like this, I've always found it to be easier to do the work in phases - so 1st phase you merge in all the easy fixes and it can be done quickly; 2nd phase you deal with another batch of changes all similar in type (e.g. implicit FLOAT to INT32 or v.v. ); and so on. So early phases deal with common (and hopefully simple) warnings, then you have a final phase which is all the strange, one-off cases where the solution perhaps isn't so clear, and each one has to be examined to make sure it's not changing the meaning of the code.

Of course, I'm not doing the merging this time, so that may not be the best way to work. Anyway, once I'm finished the first phase, I'll send a zip containing all the modified sources on to Ben. Then you can merge in little stages, or wait until they're mostly done and then do a single big merge.

FYI: I'm modifying a version of the source that I downloaded since I bought the SDK last week - might not be the most current version...

#10 You're pretty much guaranteed to be out of synch with us no matter what at this moment. If you're serious, do it once we've put RC2 out, at which point the code will be a lot less likely to change (and undo your fixes).

One thing - I'm not real gung ho about having lots and lots of extra casts in the code. :-/ It seems like the end result will just be harder to read code. The other stuff all sounds good.

#11 That's fair enough about being out of synch - its fairly methodical things that need doing and it only took me a few hours yesterday to cut the 800 warnings to <200, and it doesn't look like the rest are anything really scary - another hour or two's work at most. Undoing the fixes is no big deal, as its easy to spot them and correct them again.

As I said before - the majority of the warnings are from constants written poorly, and are easy to fix. Only about 250 of the errors require casts to fix. However, as I've been doing it, I see a wide range of styles in there already. In quite a lot of places where I was adding fixes, I'd see casts in there already - almost as if some of the devs do it properly (putting casts in), and some of them don't. So I've followed the same style as is already present in the codebase. I don't think it makes it particularly harder to read, but I'll send you the changed sources later today and you can judge for yourself.

Problem is though, if you don't want the casts, then the warnings stay; if the warnings stay, then there's not really much point in me doing this - 350 warnings instead of 800 is less, but still enough that people won't be able to pick out the important warnings from the cruft. I've found 3 cases already where implicit casting is resulting in behaviour that's really not clear (and dangerous); I'll highlight them seperately as I'm not sure what the intended behaviour is.

The point is, that implicit promotions and demotions result in extra things happening that aren't clear from the code - making it explicit means that anyone reading the code knows that a promotion/demotion is happening, and that there will be consequences of that which need to be taken into account. So harder to read is maybe not the best description - sure, its more verbose, but its also clearer exactly what is happening.

#12 Okay, all L2 warnings bar the ones from gram.cc removed, I've mailed the sources to Ben G for perusal.

#13 @Chris Chapman - You just went up a notch in my book. Most people just go away when presented with the challenge. It seems you actually took the bull by the horns...

#14 Thanks, ChisC! Will review this when I go into RC2 mode.

#15 Impressive Chris... definately will have the appreciation of a lot of Torque programmers :)

#16 Chris C. - I would be greatly appreciative if you could email me with the changes as well. Great job and thanks for doing that for everyone.

#17 Thank you for all the appreciation - I'd be lying if I said I was doing it for the sake of everyone, and not just because I occasionally go into moods where I'm really pernickety about things. ;-)

While I will forward the sources on to anyone who wants them, I would add a *strong* word of caution and a disclaimer. I was as methodical and careful as I could be while doing these fixes, and I've done it many times before and know what to be careful of; however there is always a chance that a simple typo and/or misplaced bracket can subtly change the meaning of an expression. While everything in my simple test app works fine, it certainly doesn't exercise the engine fully (and my fixes were all across the engine), nor can I claim enough familiarity with the engine that I can say it works as it used to.

If you want to use the changes before they've been ratified by GG, be aware that it might change the operation of your game. I definitely wouldn't do it if you were anywhere close to a deadline. I would rather not be held responsible for any problems incurred, so you do so at your own risk (although I think the risk is minimal).

On another note, I'm afraid the Level 3 warnings are nowhere near as easy to fix. These mostly consist of signed/unsigned mismatches, and are ingrained in the engine's API. While there are occasional occurrences where its simply a local variable that's the wrong type, more often it is parameters and structure members which are mismatched, and that would require propagating *a lot* of changes and is a job I'm sad to say is probably beyond anything except a major re-write of Torque. :-(

#18 I just wanted to pipe in here as well to say thanks about the warning fixes--I'm on the same side of the discussion as you are Chris: I think that warnings simply shouldn't be there when they are a matter of mis-/no casting, etc. IMO, there are extremely few reasons why a clean compile should output anything other than "".

#19 Trouble is, signed/unsigned mismatches are very easy to introduce, but once its come up, very hard to fix. I can see why its tempting to just ignore the warnings.

Imagine the following scenario:
Variable A is put into structure C. Its always a positive value, so the coder puts it in as an unsigned int (U32)
Variable B is put into structure D. It can be negative as well, so the coder puts it in as a signed int (S32)
A, B, C and D are then used all over the code, in hundreds of places.
Some place down the line, there's a need to write functions which compare A and B.

Bang - they don't match up. Now it might be that we can assume that when the comparison happens, B is always positive (and can be cast to U32); or we might be able to assume that A is always < INT_MAX and can be safely cast to S32. However, either way you need a cast, and to be safe, an ASSERT. The compiler will warn you because what you're doing is not safe, however in almost all cases the coder can look at it and say its okay because the coder knows what the valid ranges of the variables are (or doesn't care because by the time the values get large enough to cause problems, the engine will have been running for X years). The cleanest way to fix the problem is to change either A or B to be signed or unsigned respectively, which may then introduce the same problem elsewhere, which needs fixed the same way, and so on.

Not that it matters for Torque, because the work is already done; but I got so fed up doing this over the years that I now compulsively use signed integers for everything. I rarely (if ever) need that extra 1 bit of value, but I do run with warnings as errors, and hate seeing unnecessary casts.