🔒 Locked

Warnings - should I fix them?

Started by deadstar Mar 22, 2008 at 9:26 PM 23 replies 2.8k views

Original Post

Mar 22, 2008 at 9:26 PM

My engine is now (unsurprisingly) riddled with warnings, since I'm not a brilliant programmer to say the least. I now have a collection of about 50 - 80 things like this:


warning C4244: 'return' : conversion from 'const int64_t' to 'int', possible loss of data
warning C4244: 'argument' : conversion from 'float' to 'int', possible loss of data
warning C4800: 'int' : forcing value to bool 'true' or 'false' (performance warning)
warning C4244: '=' : conversion from 'float' to 'int', possible loss of data
warning C4018: '<' : signed/unsigned mismatch

As far as I'm aware, it works 100% on both Windows and Linux. It's been tested rigorously. What kind of problems will these warnings give me, if any. Can I get away with disabling the warning output, or is this something serious I should be addressing as soon as possible? Most likely I'll go through all of them and fix them since I'd like to tidy up my code, but I was just wondering.

brandonman

Mar 22, 2008 at 9:29 PM

Those floats to ints just mean that your number now has no decimals. So float x=1 is the same as int x=1. If it were 1.1, then it would stay float. I would just suggest initializing things as ints if they are not going to involve decimals.

bschneid

Mar 22, 2008 at 9:39 PM

I'm not a brilliant programmer either, but the compiler wouldn't really generate warnings if there was nothing to warn you about. Warnings are often caused when you have a syntactically correct statement that might not do what you expect. For example, you have a couple conversions from float to int in which your numbers lose precision due to forcing a decimal to become an integer. The compiler wants to make sure you know what you are doing because it assumes that if you created a float it was because you needed more precision than an int, but here you are losing that precision. Some warnings, though most likely signs of poor programming, are harmless, but some may really screw up your application.

The best solution would be to re-think your code. Look, for example, at the use of your float. Why are you converting a float to an int? Do you really just need an int? Maybe you should redesign your function to accept floats instead? These are the sorts of things you can think about and very easily use to determine whether you can do away with your warnings. Though not necessary, fixing the warnings can only improve the maintainability and stability of your code.

deadstar

Mar 22, 2008 at 9:42 PM

Nice feedback.

I am going to fix them. Just wondering what people's take on them was.

swiftcoder

Mar 22, 2008 at 9:52 PM

Quote:
Original post by deadstar
I am going to fix them. Just wondering what people's take on them was.

A lot of larger projects have a "warning hunt" every now and again, much like a "bug hunt" where a bunch of programmers very familiar with code-base just go through and eliminate every warning that can be removed without major code restructuring.

The reason for this is typically portability - even though you currently have Windows/Linux support, you will find that those warnings become errors in newer compiler versions, compilers for embedded (consoles), etc. For instance, many programmers had a nasty shock when GCC started enforcing const-correctness.

Antheus

Mar 22, 2008 at 9:55 PM

Always set warnings to maximum possible (/W4 or -pedantic or whichever you prefer). After short while, you'll realize that majority of them are trivial.

But warnings are useful since they often expose underlying problems with design.

When you get type conversion warning, check if the conversion is really needed, or why it occurs. If you find it's necessary, use static_cast<>. When comparing signed/unsigned, it's similar. Why are you mixing those two types? It may expose some other problems which would occur under unusual circumstances.

Quote:
Can I get away with disabling the warning output

I'll close my eyes and play in traffic. If I can't see cars, they can't hit me.

Look at it this way - compiler is trying to tell you about possible problems. Why ignore it. You'll be fixing enough bugs already, at least here someone's point out where to prevent them.

Personally, for libraries and unit tests, I run dual compilation. One under MVC with maximum warnings (including those disabled by default) and disabled language extensions, the other under GCC with maximum warnings. On both, I set warnings as errors. This approach obviously doesn't work with code that requires platform specific includes (such as windows.h)

After this passes, I sometimes run static analysis on code. And it's not uncommon to get dozens to hundreds of warnings per compilation unit.

C++ is hell. Use any tool you have to point out possible problems. If anything, compiler's errors are too mild.

Trillian

Mar 22, 2008 at 9:57 PM

Quote:
warning C4244: 'return' : conversion from 'const int64_t' to 'int', possible loss of data

This one's could be dangerous, depending on the meaning of that integer. Where do you get a int64_t from, and why do you need to cast it down to an int?

Quote:
warning C4244: 'argument' : conversion from 'float' to 'int', possible loss of data

If your intentions are to cast a float to an int (you don't need the decimal part), then just add a static_cast() and the warning will diseappear.

Quote:
warning C4800: 'int' : forcing value to bool 'true' or 'false' (performance warning)

That's an easy one, I beleive you are doing something like "bool ok = x;", where "x" is an integer expression. Be explicit and change your code to "bool ok = x != 0;", if that's what you want.

Quote:
warning C4018: '<' : signed/unsigned mismatch

I've seen this often when comparing size_t to int. Make sure you're using the appropriate data types. If you don't want to change your data types, use a static_cast<>.

Antheus

Mar 22, 2008 at 10:03 PM

Quote:
Original post by Trillian

Quote:
warning C4800: 'int' : forcing value to bool 'true' or 'false' (performance warning)

That's an easy one, I beleive you are doing something like "bool ok = x;", where "x" is an integer expression. Be explicit and change your code to "bool ok = x != 0;", if that's what you want.

Missed that one. That's an important one, since it can reveal a typo.

bool foo(int x){	return x=17;}int main(){	foo(5);}

foo should say:

return x==17;

swiftcoder

Mar 22, 2008 at 10:05 PM

Quote:
Original post by Trillian
Quote:
warning C4018: '<' : signed/unsigned mismatch

I've seen this often when comparing size_t to int. Make sure you're using the appropriate data types. If you don't want to change your data types, use a static_cast<>.

That is more dangerous than it looks at first blush - size_t is not in any way guaranteed to be the same size as int, especially on 64-bit platforms. Prefer size_t whenever dealing with std::containers.

deadstar

Mar 22, 2008 at 10:18 PM

Quote:
warning C4244: 'return' : conversion from 'const int64_t' to 'int', possible loss of data

This one isn't mine, it's from the ffmpeg includes.

Quote:
warning C4800: 'int' : forcing value to bool 'true' or 'false' (performance warning)

And this one is when I register a Lua function to a C++ function that takes a bool. I might alter the function to take an int instead.

Promit

Mar 22, 2008 at 10:22 PM

Fix all of them, and then turn on warnings as errors. That's the only way you'll ever learn.

deadstar

Mar 22, 2008 at 10:24 PM

Quote:
Original post by Promit
Fix all of them, and then turn on warnings as errors. That's the only way you'll ever learn.

I plan to, but how would I sort out warnings that aren't in my code? That ffmpeg header chucks out a fair few.

ToohrVyk

Mar 23, 2008 at 2:39 AM

You can disable warnings for a region in your code (such as the include directive).

deadstar

Mar 23, 2008 at 12:05 PM

Good.

And what would be the difference in doing the following:

Something = static_cast<int>lua_tonumber(LuaState, 3);

Something = (int)lua_tonumber(LuaState, 3);

Both seem to work. Does one have benefits over the other?

Mike.Popoloski

Mar 23, 2008 at 12:11 PM

The first is the C++ version, while the second is the old C-style version. You should prefer the first, since it is more explicit in what it is doing. With the C version, you could end up doing reinterpret casts, const casts, and static casts, whereas with the C++ versions you are explicit about what you want it to do, which means it's easier for the compiler to help catch errors for you.

SpreeTree

Mar 23, 2008 at 12:33 PM

Quote:
Original post by swiftcoder
A lot of larger projects have a "warning hunt" every now and again, much like a "bug hunt" where a bunch of programmers very familiar with code-base just go through and eliminate every warning that can be removed without major code restructuring.

On a project of any reasonable size, there should never be a situation where time out needs to be taken to remove all the warnings. As soon as a warning appears, it needs to be removed.

With warnings as errors, and a continuous build process, there should never be a situation where a platform is ignored and then needed to be tackled in bulk.

When a new or updated compiler is released, there will generally be one or two people tasked with testing and porting the code, removing the warnings/errors again as they appear.

A "warning hunt" is a sure sign that the development of the process is not being managed correctly.

For times when you are using a library that causes warnings internally, you either need to selectivly remove them or, if possible, contact the author and flag a bug with their project.

Spree

deadstar

Mar 23, 2008 at 12:48 PM

Ok I've started the task of warning-killing. A few problems are giving me a headache, mostly my Vector3 and Vertex classes.

It seems a little daft to have a copy constructor for the class to accept integers, so how would I get around something like this:

		for (int x = 0; x < Width - 3; x++)		{			for (int z = 0; z < Height - 3; z++)			{				*TempVertex = SYM_VERTEX(x, MapData[x * Width + z], z);

The MapData[] is an array of unsigned ints. The x and z variables in the 'for' loop are ints (use floats for a 'for' loop?) and the Vertex class accepts floats.

That one line alone generates three warnings, complaining about the conversion from int to float.

I could use static_cast<>, but that would mean expanding that single line onto three lines, and the same for approx 30 similar cases. That would make a mess of an entire page of source.

Is this necessary? As far as I can see, the data types chosen are appropriate. The Vector3 and Vertex classes are used throughout the entire project, and 99% of uses involve floats. Changing the MapData[] to a float array would be unnecessary since only unsigned integers will be stored in it.

How do people get around issues like this? Use static_cast<> all over the shop?

TheTroll

Mar 23, 2008 at 1:30 PM

for (float x = 0.0f; x < Width - 3.0f; x+= 1.0f)
{
for (float z = 0.0f; z < Height - 3.0f; z += 1.0f)

There is no reason you can not use floats for this.

theTroll

Antheus

Mar 23, 2008 at 2:05 PM

Quote:
Original post by TheTroll
for (float x = 0.0f; x < Width - 3.0f; x+= 1.0f)
{
for (float z = 0.0f; z < Height - 3.0f; z += 1.0f)

There is no reason you can not use floats for this.

Aside from killing the performance of a trivial for loop, no.

Quote:
That one line alone generates three warnings, complaining about the conversion from int to float

Yes, and it's appropriate. x coordinates are uniformly spaced, your float coordinates won't be. The deviation will be minimal, but it'll be there. So compiler is right - you are losing accuracy, although it might not matter.

Without knowing the entire context, using static_cast<>, or even (float) is appropriate here.

There's also alternatives (another constructor for creating vertices from a map):

Vertex::Vertex(unsigned int mapData[], int x_, int y_, int z_)  : x(static_cast<float>(x_)  , y(static_cast<float>(mapData[x_*Width+z_])  , z(static_cast<float>(z_){}

mapData doesn't supply width though, so you'd need to pass that in.

If you change Map into a class (encapsulate construction):

struct Map {  ...  const Vertex & makeVertex(unsigned int x, unsigned int y, unsigned z)  {    float xv(x);    float yv(mapData[x*Width+z]);    float zv(z);    return Vertex(xv, yv, zv);      }private:  unsigned int width;  unsigned int height;  std::vector<unsigned int> mapData;};

This solution is somewhat prettiest design-wise since it hides all the details.

And there's probably others.

Stab-o-tron

Mar 23, 2008 at 5:11 PM

I don't know how it is other places, but where I work, we always compile with the "treat warnings as errors" compile option. It can be a bit of a hassle to begin with, but once you get into the habit, it's not so bad.

Judging by some of the comments that other posters have made, it seems that this s pretty common, so it's a good habit to get into.

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.