Compile warnings report

[dkulis]

Some compile warining stats (using VS2003.NET on Win2K Pro):

Code: Select all

count warning
41    C4018: '<'/'<='/'>'/'>=' : signed/unsigned mismatch
30    C4800: 'const int'/'int' : forcing value to bool 'true' or 'false' (performance warning)
12    C4244: conversion with possible loss of data
10    C4101: 'e' : unreferenced local variable
3     C4355: 'this' : used in base member initializer list
1     C4146: unary minus operator applied to unsigned type, result still unsigned

Total 97 warnings.

Anything I can help to get those removed?

At lest some of the "C4018 signed/unsigned mismatch" are result of (unsigned) size_t beeing used for mapsizes while coordinates are (unsigned) int.

[Dave]

Some compile warining stats (using VS2003.NET on Win2K Pro):

Code: Select all

[b]count warning[/b]
41    C4018: '<'/'<='/'>'/'>=' : signed/unsigned mismatch
30    C4800: 'const int'/'int' : forcing value to bool 'true' or 'false' (performance warning)
12    C4244: conversion with possible loss of data
10    C4101: 'e' : unreferenced local variable
3     C4355: 'this' : used in base member initializer list
1     C4146: unary minus operator applied to unsigned type, result still unsigned

Total 97 warnings.

Anything I can help to get those removed?

IMO most of these warnings are silly.

C4018: should possibly use a cast, but really, I don't see the problem with making comparisons between signed and unsigned types.
C4800: implicit conversion from an int to a bool is a very common C/C++ idiom. I don't understand why the compiler would warn on it.
C4244: may be a problem; will look into it.
C4101: this one is just silly on the part of the compiler. You want to name an exception in a catch block so you can refer to it in the debugger. It is 'possible' to just place 'e;' in the catch block to silence the warning, but imo it's so silly that the compiler even warns you of this that I generally refuse to.
C4355: Often a class has to pass a reference to itself to one of its member's constructors. This is how callback mechanisms often work.
C4146: hmm...where is that occurring at?

David

[dkulis]

C4244: may be a problem; will look into it.
C4101: this one is just silly on the part of the compiler. You want to name an C4146: hmm...where is that occurring at?

See attachement of the original post

[Dave]

C4244: may be a problem; will look into it.
C4101: this one is just silly on the part of the compiler. You want to name an C4146: hmm...where is that occurring at?

'

See attachement of the original post

Ahh oops

Anyhow, 4101 should probably use explicit casts.

4146 is because of (num > 0 ? num : -num). 'num' is of varying types, since this is in a function template. Clearly this isn't a problem, but I don't know of any way to silence the warning without using lots of template nastiness.

David

[dkulis]

C4018: should possibly use a cast, but really, I don't see the problem with making comparisons between signed and unsigned types

BTW, why are you using size_t for unsigned? As I understand, it is not intented to be as general unsigned int.

And why bother with unsigned at all? In most/all cases where you need unsigned, big values (i.e. negatives using plain signed types) are as wrong as negative (e.g. -3 is as bad as 4294967293)

And one more stupid C++ question.
What does this syntax "size_t(expr)" means:

Code: Select all

in video.cpp line 160:

		if(size_t(rect.x) >= fb->w) {
			return;
		}

It's not a cast, not a function call, looks like new object - but why?

BTW, this is one of places where you get warning C4018: '>=' : signed/unsigned mismatch.

[Dave]

C4018: should possibly use a cast, but really, I don't see the problem with making comparisons between signed and unsigned types

BTW, why are you using size_t for unsigned? As I understand, it is not intented to be as general unsigned int.

size_t is generally used for anything that represents the size of something, or must be compared with something that represents the size of something.

And why bother with unsigned at all? In most/all cases where you need unsigned, big values (i.e. negatives using plain signed types) are as wrong as negative (e.g. -3 is as bad as 4294967293)

(1) it's easier to check that the values are legal.

Code: Select all

if(i < array.size())

instead of,

Code: Select all

if(i >= 0 && i < array.size())

(2) it makes the code more readable by indicating the intended usage of the variable.

And one more stupid C++ question.
What does this syntax "size_t(expr)" means:

Code: Select all

in video.cpp line 160:

		if(size_t(rect.x) >= fb->w) {
			return;
		}

It's not a cast, not a function call, looks like new object - but why?

Yes, it's constructing a new object of type size_t which it's using to do the comparison. But, it should probably not use the object construction, since fb->w is indeed a signed type (this is rather confusing in SDL, the 'h' and 'w' fields of SDL_Rects are unsigned, while of SDL_Surfaces are signed).

Constructing a new object like this is safer than doing a C-style cast, since a C-style cast will allow all sorts of weird conversions (e.g. it would allow it if rect.x was a pointer, while the construction will cause an error).

The alternative that uses a cast is to use a C++ static_cast like this:

if(static_cast<size_t>(rect.x) >= fb->w) {

But, that is kinda verbose...

David

[dkulis]

size_t is generally used for anything that represents the size of something, or must be compared with something that represents the size of something.

As I understood from my C++ docs, intended meaning is even narower, it's the size of objects in memory... (i.e. hardwarish thingy)



Sorry, had to ask

Yes, it's constructing a new object of type size_t which it's using to do the comparison.

This new obeject is allocated on stack? And will optimizer eventually get rid of it?

[Dave]

size_t is generally used for anything that represents the size of something, or must be compared with something that represents the size of something.

As I understood from my C++ docs, intended meaning is even narower, it's the size of objects in memory... (i.e. hardwarish thingy)

Well, I often use 'size_t' when the 'proper' way to do things would be e.g. std::vector<int>::size_type -- because I feel that that is tediously verbose.

Sorry, had to ask

Yes, it's constructing a new object of type size_t which it's using to do the comparison.

This new obeject is allocated on stack? And will optimizer eventually get rid of it?

[/quote]

Well, it's not allocated on the heap or free store, if that's what you're asking. Possibly on the stack, possibly in a CPU register. Who knows. I have given up on speculating as to what optimizers will or will not do long ago.

David

[Darth Fool]

I understand most of your objections to paying too much attention to the warnings. Still, it would be nice if v1.0 compiled without warnings.

[Dave]

I understand most of your objections to paying too much attention to the warnings. Still, it would be nice if v1.0 compiled without warnings.

We support numerous platforms. Which of those platforms did you want it to compile on without warnings?

Also, some warnings cannot be sanely prevented afaik. For instance, passing the 'this' pointer to a member in the constructor is a common C++ idiom, and I know of no other good way to do it. If a compiler wants to warn us about this, what should we do about it?

David