Sign in to follow this  
deadstar

Warnings - should I fix them?

Recommended Posts

deadstar    536
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.

Share this post


Link to post
Share on other sites
brandonman    102
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.

Share this post


Link to post
Share on other sites
bschneid    126
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.

Share this post


Link to post
Share on other sites
swiftcoder    18426
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.

Share this post


Link to post
Share on other sites
Antheus    2409
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.

Share this post


Link to post
Share on other sites
Trillian    410
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<int>() 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<>.

Share this post


Link to post
Share on other sites
Antheus    2409
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;

Share this post


Link to post
Share on other sites
swiftcoder    18426
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.

Share this post


Link to post
Share on other sites
deadstar    536
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.


Share this post


Link to post
Share on other sites
deadstar    536
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.

Share this post


Link to post
Share on other sites
deadstar    536
Good.

And what would be the difference in doing the following:


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



or


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



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

Share this post


Link to post
Share on other sites
Mike.Popoloski    3258
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.

Share this post


Link to post
Share on other sites
SpreeTree    396
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

Share this post


Link to post
Share on other sites
deadstar    536
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?

Share this post


Link to post
Share on other sites
TheTroll    883
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

Share this post


Link to post
Share on other sites
Antheus    2409
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.

Share this post


Link to post
Share on other sites
Stab-o-tron    332
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.

Share this post


Link to post
Share on other sites
exwonder    100
Quote:
Original post by Mike.Popoloski
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.


The C-style casts are completely safe for casts between basic types or removing signed-ness, etc. The split functionality for C++ casts is a good idea, except that they intentionally have an annoying syntax because the language creators believed that casts should never be used. A small example of personal style considerations getting in the way of language usefulness.

For casts between integral or floating point types I always use C-style casts or the constructor cast syntax, both of which are deprecated in C++. There's no point in being pedantic about it when it's much cleaner and totally safe if you restrict your usage of C-style casting to simple conversions between builtin types.

For casting pointers, though, you should always use the C++ style casts unless you're absolutely positive that the C-style casts work as expected in your specific case.

Share this post


Link to post
Share on other sites
exwonder    100
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.

theTroll


Double post, I apologize, but this isn't recommended at all. Floating point comparisons are slow on many processors, so if you feel you *need* to do something like this it's best to get into the habit of doing something a little different:

float f = 0.0f;
for (int i = 0; i < Width; i++, f+= 1.0f)
{
// ...
}

Casting the ints to floats to pass to the function is fine, but if you can cleanly do something like the above it's likely to be much much faster than casting on any processor where load-hit-store is an issue... and probably slightly faster everywhere else.

Share this post


Link to post
Share on other sites
taby    1265
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 could not agree more.

Also, if you're using MSVC++, turn off those dumbass warnings about the C library being insecure. It's hilarious that Microsoft tries to cram their "replacement" library down the throats of programmers -- and yet there are still buffer overflow bugs in their new software. Apparently either a) their programmers don't use the "replacement" library, or b) the "replacement" library is no more secure than the C library. Either way, using their "replacement" library immediately renders one's code compatible only with MSVC++, which is just plain bad since it's avoidable in this case.

Share this post


Link to post
Share on other sites
exwonder    100
Quote:
Original post by taby
Also, if you're using MSVC++, turn off those dumbass warnings about the C library being insecure. It's hilarious that Microsoft tries to cram their "replacement" library down the throats of programmers -- and yet there are still buffer overflow bugs in their new software. Apparently either a) their programmers don't use the "replacement" library, or b) the "replacement" library is no more secure than the C library. Either way, using their "replacement" library immediately renders one's code compatible only with MSVC++, which is just plain bad since it's avoidable in this case.

Their "replacement library" has been submitted for standardization, and is a decent idea, though I don't like the implementation of the exception handler.

Anyway, might as well learn to live with the *_s functions (or embrace and use them), because they'll probably be standard C at some point in the future. You can macro them to work on other platforms without a whole lot of trouble.

Really, though, unless you absolutely know what you're doing the C string manipulation functions are worth avoiding. Professional programmers often screw up and use them in a way that allows one (or more) byte buffer overflows. Probably because strncpy, strncat, snprintf all work slightly differently and require different precautions to protect from buffer overflow. The _s versions might not be the perfect solution, but at least someone is trying to address the problem.

Share this post


Link to post
Share on other sites
bmanruler    122
I know I am not a great programmer, so in general I assume the compiler people know much more about C++ than me. Turning on level 4 warnings is annoying at first, but will save you lots of headache later on.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this