Compiler warnings
by Gerald Fishel · in Torque 3D Professional · 10/12/2011 (12:51 am) · 14 replies
So as I prepare to work on my next T3D project, I have been starting with 1.1 and creating a CMake-based build system to build it, to make it easier to manage certain cross-platform things.
Fairly early I discovered that I get 10 billion compiler warnings when I compile the T3D core from my own build script. A look at the Visual Studio project files shows why I never saw these warnings before. You have a long list of compiler warnings disabled. This is bad mojo! Please fix the warnings instead of hiding them. I realize that a lot of times you get warnings for things that are safe and it's tempting to just disable them, but hiding the warning messages covers up things that are not safe, like this:
I'm not sure if this code is ever actually used in the engine, but if it is it is certainly not going to set any color components to 0.5f in a ColorI. The compiler can tell you about this if you don't tell it not to ;)
This could explain a lot. I have a feeling I'm going to find a bunch more of this stuff as I wade through the rest of these warning messages. Either way, that's a terrible practice.
Fairly early I discovered that I get 10 billion compiler warnings when I compile the T3D core from my own build script. A look at the Visual Studio project files shows why I never saw these warnings before. You have a long list of compiler warnings disabled. This is bad mojo! Please fix the warnings instead of hiding them. I realize that a lot of times you get warnings for things that are safe and it's tempting to just disable them, but hiding the warning messages covers up things that are not safe, like this:
/// The water fog settings.
struct WaterFogData
{
F32 density;
F32 densityOffset;
F32 wetDepth;
F32 wetDarkening;
ColorI color;
PlaneF plane;
F32 depthGradMax;
WaterFogData()
{
density = 0.0f;
densityOffset = 0.0f;
wetDepth = 0.0f;
wetDarkening = 0.0f;
color.set( 0.5f, 0.5f, 0.5f, 1.0f );
plane.set( 0.0f, 0.0f, 1.0f );
depthGradMax = 0.0f;
}
};I'm not sure if this code is ever actually used in the engine, but if it is it is certainly not going to set any color components to 0.5f in a ColorI. The compiler can tell you about this if you don't tell it not to ;)
This could explain a lot. I have a feeling I'm going to find a bunch more of this stuff as I wade through the rest of these warning messages. Either way, that's a terrible practice.
#3
10/12/2011 (10:06 am)
Been going through the warnings one by one and already found at least a dozen hidden bugs waiting to happen. Not to mention some pretty silly design decisions. Apparently torque developers just choose S32 and U32 at random when they need an integer, and sometimes when they need a floating point, and as long as the compiler doesn't complain - which it won't with every warning disabled - screw it.
#4
Yeah, I'll probably have nightmares about the things I've been seeing while wading through this stuff. I've found long chains of calculations that keep mixing and matching floats, signed and unsigned integers and back again that can't possibly do anything close to what they're supposed to do. Not that you could ever figure out what they're supposed to do. Just like the compiler warnings, if you can't prove it's wrong it's gotta be right I guess.
Seriously, with all of the bugs and particularly the precision issues that the engine keeps producing, GG really needs to take a couple of GOOD programmers and set them loose on the engine for a few days just to fix all of the compiler warnings. (Not the guys that have apparently tried to suppress compiler warnings in the long lost past by just randomly throwing casts in front of variables until the compiler shuts up.)
My favorite is the dozens of these strewn throughout the code:
if (bool(someVar) == false)
to test for null pointers! Great stuff.
I know it's painful to lose the man hours for this type of thing, but that little sacrifice in the near term can save you months of work and add years to your life, not to mention helping you provide a product that actually works like it's supposed to work... and works like it looks like it's supposed to work!
10/13/2011 (10:58 pm)
Disabling compiler warnings, it's the stuff that nightmares are made of!Yeah, I'll probably have nightmares about the things I've been seeing while wading through this stuff. I've found long chains of calculations that keep mixing and matching floats, signed and unsigned integers and back again that can't possibly do anything close to what they're supposed to do. Not that you could ever figure out what they're supposed to do. Just like the compiler warnings, if you can't prove it's wrong it's gotta be right I guess.
Seriously, with all of the bugs and particularly the precision issues that the engine keeps producing, GG really needs to take a couple of GOOD programmers and set them loose on the engine for a few days just to fix all of the compiler warnings. (Not the guys that have apparently tried to suppress compiler warnings in the long lost past by just randomly throwing casts in front of variables until the compiler shuts up.)
My favorite is the dozens of these strewn throughout the code:
if (bool(someVar) == false)
to test for null pointers! Great stuff.
I know it's painful to lose the man hours for this type of thing, but that little sacrifice in the near term can save you months of work and add years to your life, not to mention helping you provide a product that actually works like it's supposed to work... and works like it looks like it's supposed to work!
#5
Now to get all of the third-party stuff in the CMake build system... and then investigate the 117 potential bugs that I logged along the way...
10/14/2011 (5:27 pm)
Whew. 2 days later I finally got it to compile with almost no warnings. Just a few that are in some template soup in EngineAPI.h and some macros that I plan on replacing with template code.Now to get all of the third-party stuff in the CMake build system... and then investigate the 117 potential bugs that I logged along the way...
#6
10/18/2011 (12:48 pm)
I think this kind of thing deffinately deserves a good rant. I am not a skilled programer by any stretch of the imagination and know just enough to be dangerous but every programming class i have ever taken has always started with treat warnings the same way you treat errors. A policy of sweeping them under the rug is some serious bad ju ju, and a sure fire way to lose additional consumer confidence.
#7
Aside from all of the potentially buggy behavior it's covering up, it's also hiding a lot of performance issues that are immediately apparent when the warnings are enabled, because almost every significant operation in the engine is mixing 32-bit and 64-bit floating point numbers, which results in additional CPU overhead to convert between them. While some of that is unavoidable, most of it isn't and is easily fixed if you realize it's happening. In many cases it's just a matter of adding the "f" to the end of a constant value.
I finally got it to build completely via CMake with all of the third party stuff, and without any warnings in the engine (not worried about the warnings in the third party libs, since I don't want to mess with those.) I've gained about a 3% performance boost right off the bat just by fixing a bunch of those unnecessary floating point conversions. Not huge, but significant in a game engine.
On the other hand, this has been a fairly educational experience for me as far as the engine goes. I see that I can probably gain another 5-10%, maybe even more, in many spots by replacing all of the naive loops, sorts and searches with STL algorithms.
10/18/2011 (1:14 pm)
Exactly.Aside from all of the potentially buggy behavior it's covering up, it's also hiding a lot of performance issues that are immediately apparent when the warnings are enabled, because almost every significant operation in the engine is mixing 32-bit and 64-bit floating point numbers, which results in additional CPU overhead to convert between them. While some of that is unavoidable, most of it isn't and is easily fixed if you realize it's happening. In many cases it's just a matter of adding the "f" to the end of a constant value.
I finally got it to build completely via CMake with all of the third party stuff, and without any warnings in the engine (not worried about the warnings in the third party libs, since I don't want to mess with those.) I've gained about a 3% performance boost right off the bat just by fixing a bunch of those unnecessary floating point conversions. Not huge, but significant in a game engine.
On the other hand, this has been a fairly educational experience for me as far as the engine goes. I see that I can probably gain another 5-10%, maybe even more, in many spots by replacing all of the naive loops, sorts and searches with STL algorithms.
#8
10/19/2011 (9:46 am)
Intrigued by your performance increase Gerald, as well as your expectation of further improvement. That's nice to know... guess I'll be 'fixing' those warnings as well.
#9
Going through now and doing some analysis on the use of containers. It seems like they're using vectors for everything except where a map is needed, and calling the vector a list or a set. Doing random insertions and deletions on vectors is hideous. Even having such a thing as a push_front method on a vector class makes me shudder :P
10/19/2011 (12:09 pm)
You gotta be careful when doing it. I actually had to do it twice, after the first time I got it to build and it would hang like crazy, so I went through again more carefully. Those horrible reverse iterations are nasty if you fix a signed integer that really should be an unsigned integer, and then find out they're using it in a reverse iteration somewhere that checks if it's >= 0. I find myself wondering if they changed them to signed just so they could do that. But it really just seems like they're choosing S32 and U32 at random.Going through now and doing some analysis on the use of containers. It seems like they're using vectors for everything except where a map is needed, and calling the vector a list or a set. Doing random insertions and deletions on vectors is hideous. Even having such a thing as a push_front method on a vector class makes me shudder :P
#10
One of the suggestions for Community Contributions involves cleaning up the compiler warnings so I decided to take a stab at it and promptly ran into an absolute nightmare. There are thousands of warnings regarding signed/unsigned mismatches alone and many of those lead to some very old, very scary code. S32 and U32 (and int in some rare places!) are used as though they're completely interchangeable which is a bad assumption.
07/05/2013 (11:30 pm)
This deserves a bump. One of the suggestions for Community Contributions involves cleaning up the compiler warnings so I decided to take a stab at it and promptly ran into an absolute nightmare. There are thousands of warnings regarding signed/unsigned mismatches alone and many of those lead to some very old, very scary code. S32 and U32 (and int in some rare places!) are used as though they're completely interchangeable which is a bad assumption.
#11
07/06/2013 (12:23 am)
Definitely. Maybe we need to figure out how to break it into smaller bits and start working on those. The signed/unsigned stuff is a major can of worms, but something more actionable is all the redefinitions of i inside for loops.Quote:but seems that the overall opinion is that if it compiles then it works.That'd be acceptable if Torque were written in Haskell ;P.
#12
The signed/unsigned mismatches are doubly bad because not only is the engine being bogged down by needless casts between variable types, a lot of code seems to be built around the assumption that ints are all the same. You end up in this horrible situation of not being able to fix something small without breaking something larger.
Yuck.
07/06/2013 (1:01 am)
Breaking it into more manageable tasks is about the only way I can see this being fixed. The sheer number of warnings is overwhelming. The signed/unsigned mismatches are doubly bad because not only is the engine being bogged down by needless casts between variable types, a lot of code seems to be built around the assumption that ints are all the same. You end up in this horrible situation of not being able to fix something small without breaking something larger.
Yuck.
#13
07/06/2013 (2:48 am)
First step: we should probably see where the project generator squashes those errors and not do that :P.
#14
I spent a couple more hours last night working on just the signed/unsigned mismatches but I still have a ways to go before they're all gone. A lot of them are easy fixes but some lead you into this weird chicken-and-egg scenario.
The Vector class is a good example. Its size() method returns an S32, except vectors should never have a negative size so it seems like a simple fix. Except 'fixing' it now causes a warning on half the for loops in the engine because they've all assumed that Vector::size() will return a signed int and so they've used a signed int as an iterator. And fixing the iterators isn't always straightforward because occasionally they -need- to be signed.
If you leave Vector::size() alone you've still got warnings on the -other- half of the for loops that (correctly!) iterate with an unsigned int. You can get rid of these warnings by casting either size() or the iterator but that just feels like explicitly doing what the compiler was implicitly doing before.
At some point around 3am doing a find & replace to convert all the U32s in the engine to S32s started sounding like a good idea. What could possibly go wrong? :P
07/06/2013 (10:34 am)
Yep. I spent a couple more hours last night working on just the signed/unsigned mismatches but I still have a ways to go before they're all gone. A lot of them are easy fixes but some lead you into this weird chicken-and-egg scenario.
The Vector class is a good example. Its size() method returns an S32, except vectors should never have a negative size so it seems like a simple fix. Except 'fixing' it now causes a warning on half the for loops in the engine because they've all assumed that Vector::size() will return a signed int and so they've used a signed int as an iterator. And fixing the iterators isn't always straightforward because occasionally they -need- to be signed.
If you leave Vector::size() alone you've still got warnings on the -other- half of the for loops that (correctly!) iterate with an unsigned int. You can get rid of these warnings by casting either size() or the iterator but that just feels like explicitly doing what the compiler was implicitly doing before.
At some point around 3am doing a find & replace to convert all the U32s in the engine to S32s started sounding like a good idea. What could possibly go wrong? :P
Associate Michael Hall
Distracted...