I recall (what must by now be a pretty long time ago) that Twisol once started to fix up MUSHclient warnings in recent compilers. But when I spoke to him it came up that Nick apparently never merged any of the fixes he brought up, and by now it would probably be a lost cause given the amount of new versions that have come out.
Since I hope to contribute, I'd like to pick up on that warnings-squashing project, since it is pretty much impossible to see the trees in the forest of warnings that even a partial build tends to throw up.
Thus, my question to Nick is - are you willing to bring such fixes into your master, or did you have some sort of issues with it?
I have zero intention of forking MUSHclient, and if anything 'new' that I code up can't be merged due to me fixing those warnings, it'd be a pretty useless endeavour, so I'd prefer to know before I spend too much time on such a project.
So far, I put up a few branches on github in case any of you want to check out compatibility with VC++6, but it is all trivial thus far.
So this must some other reality, the one where I ignore all suggested changes.
What I *am* resisting is some of the refactoring proposals where functions that currently work, are just being rewritten with a view to making them "nicer". This just involves work in writing, analyzing, testing, and checking.
In fact I had already committed your changes before I read this thread:
Sorry if I wasn't totally clear, Worstje; the problem was that I spent so much time doing warnings mixed with stylistic cleanups, and it was such a mess that Nick really couldn't pull things in like that. It was only worthwhile to just re-fix actual bugs I brought up. And since the warnings are just that - warnings - it wasn't important for him to spend the time plucking these cleanups out of my commit chain.
[EDIT] Nick, you mentioned ff972aec6b twice in that set of links. ;)
[EDIT] And unless I'm quite mistaken, you actually made the 91abdc79ff change; I had simply merged it into my branch, and you later merged my branch into your branch.
If you stick to eliminating compiler warnings that would be great, because then when I update the compiler one day, it will work for me too. :)
So that would be appreciated. And of course if in the process of browsing you spot bugs, well I am glad to hear about them too. Twisol has found a few so far, and their fixes are gratefully received and acknowledged.
Nick Gammon said: If you stick to eliminating compiler warnings that would be great, because then when I update the compiler one day, it will work for me too. :)
Nick, you mentioned ff972aec6b twice in that set of links. ;)
And unless I'm quite mistaken, you actually made the 91abdc79ff change; I had simply merged it into my branch, and you later merged my branch into your branch.
Fixed double posting - I must have gone through the list twice, or in two different ways (I think I searched for Twisol and then your real name).
As for who made the change, it was tagged "Twisol (author)". I think we discussed a while back that this was a bit misleading, but you got the credit.
My apologies for my claim of you never merging anything, Nick. I didn't doublecheck against the log of commits done and obviously misunderstood Twisol. :)
And I hadn't spotted that commit yet. Still finding my way on Github, obviously, but glad to see you're more on top than I thought you were. Kudos!
I do intend to stick to fixing compiler warnings only on my vs2010_warnings branch.
And I hadn't spotted that commit yet. Still finding my way on Github, obviously, but glad to see you're more on top than I thought you were. Kudos!
Well, only just. :)
I had to fetch and then merge it. Should be faster next time.
My compile gives me:
--------------------Configuration: MUSHclient - Win32 Debug--------------------
Compiling...
globalregistryoptions.cpp
Linking...
nafxcwd.lib(wincore.obj) : error LNK2005: _g_fMultiMonInitDone already defined in winplace.obj
nafxcwd.lib(wincore.obj) : error LNK2005: _g_pfnEnumDisplayMonitors already defined in winplace.obj
nafxcwd.lib(wincore.obj) : error LNK2005: _g_pfnGetMonitorInfo already defined in winplace.obj
nafxcwd.lib(wincore.obj) : error LNK2005: _g_pfnMonitorFromPoint already defined in winplace.obj
nafxcwd.lib(wincore.obj) : error LNK2005: _g_pfnMonitorFromRect already defined in winplace.obj
nafxcwd.lib(wincore.obj) : error LNK2005: _g_pfnMonitorFromWindow already defined in winplace.obj
nafxcwd.lib(wincore.obj) : error LNK2005: _g_pfnGetSystemMetrics already defined in winplace.obj
nafxcwd.lib(wincore.obj) : error LNK2005: _InitMultipleMonitorStubs already defined in winplace.obj
nafxcwd.lib(wincore.obj) : error LNK2005: _xGetSystemMetrics@4 already defined in winplace.obj
nafxcwd.lib(wincore.obj) : error LNK2005: _xMonitorFromPoint@12 already defined in winplace.obj
nafxcwd.lib(wincore.obj) : error LNK2005: _xMonitorFromRect@8 already defined in winplace.obj
nafxcwd.lib(wincore.obj) : error LNK2005: _xMonitorFromWindow@8 already defined in winplace.obj
nafxcwd.lib(wincore.obj) : error LNK2005: _xGetMonitorInfo@8 already defined in winplace.obj
nafxcwd.lib(wincore.obj) : error LNK2005: _xEnumDisplayMonitors@16 already defined in winplace.obj
nafxcwd.lib(wincore.obj) : warning LNK4006: _g_fMultiMonInitDone already defined in winplace.obj; second definition ignored
nafxcwd.lib(wincore.obj) : warning LNK4006: _g_pfnEnumDisplayMonitors already defined in winplace.obj; second definition ignored
nafxcwd.lib(wincore.obj) : warning LNK4006: _g_pfnGetMonitorInfo already defined in winplace.obj; second definition ignored
nafxcwd.lib(wincore.obj) : warning LNK4006: _g_pfnMonitorFromPoint already defined in winplace.obj; second definition ignored
nafxcwd.lib(wincore.obj) : warning LNK4006: _g_pfnMonitorFromRect already defined in winplace.obj; second definition ignored
nafxcwd.lib(wincore.obj) : warning LNK4006: _g_pfnMonitorFromWindow already defined in winplace.obj; second definition ignored
nafxcwd.lib(wincore.obj) : warning LNK4006: _g_pfnGetSystemMetrics already defined in winplace.obj; second definition ignored
nafxcwd.lib(wincore.obj) : warning LNK4006: _InitMultipleMonitorStubs already defined in winplace.obj; second definition ignored
nafxcwd.lib(wincore.obj) : warning LNK4006: _xGetSystemMetrics@4 already defined in winplace.obj; second definition ignored
nafxcwd.lib(wincore.obj) : warning LNK4006: _xMonitorFromPoint@12 already defined in winplace.obj; second definition ignored
nafxcwd.lib(wincore.obj) : warning LNK4006: _xMonitorFromRect@8 already defined in winplace.obj; second definition ignored
nafxcwd.lib(wincore.obj) : warning LNK4006: _xMonitorFromWindow@8 already defined in winplace.obj; second definition ignored
nafxcwd.lib(wincore.obj) : warning LNK4006: _xGetMonitorInfo@8 already defined in winplace.obj; second definition ignored
nafxcwd.lib(wincore.obj) : warning LNK4006: _xEnumDisplayMonitors@16 already defined in winplace.obj; second definition ignored
Creating library .\WinDebug/MUSHclient.lib and object .\WinDebug/MUSHclient.exp
.\WinDebug/MUSHclient.exe : fatal error LNK1169: one or more multiply defined symbols found
Error executing link.exe.
MUSHclient.exe - 15 error(s), 14 warning(s)
If I comment out this line it works:
#define COMPILE_MULTIMON_STUBS
So you need to find a way of making a compiler-version test to put around it. Or I can try to find it.
Nick, Worstje, what if you replace 'multimon.h' with 'windows.h'? I suggested this in another thread, and it was the solution I used when I first started with the source. Since you both have different windows.h's anyways (I'm pretty sure), I think it should work without too much pain.
Nick, Worstje, what if you replace 'multimon.h' with 'windows.h'? I suggested this in another thread, and it was the solution I used when I first started with the source. Since you both have different windows.h's anyways (I'm pretty sure), I think it should work without too much pain.
Doesn't work.
--------------------Configuration: MUSHclient - Win32 Debug--------------------
Compiling...
winplace.cpp
C:\source\mushclient\winplace.cpp(46) : error C2065: 'HMONITOR' : undeclared identifier
C:\source\mushclient\winplace.cpp(46) : error C2146: syntax error : missing ';' before identifier 'hMonitor'
C:\source\mushclient\winplace.cpp(46) : error C2065: 'hMonitor' : undeclared identifier
C:\source\mushclient\winplace.cpp(47) : error C2065: 'MONITORINFO' : undeclared identifier
C:\source\mushclient\winplace.cpp(47) : error C2146: syntax error : missing ';' before identifier 'mi'
C:\source\mushclient\winplace.cpp(47) : error C2065: 'mi' : undeclared identifier
C:\source\mushclient\winplace.cpp(52) : error C2065: 'MonitorFromRect' : undeclared identifier
C:\source\mushclient\winplace.cpp(52) : error C2065: 'MONITOR_DEFAULTTONEAREST' : undeclared identifier
C:\source\mushclient\winplace.cpp(57) : error C2228: left of '.cbSize' must have class/struct/union type
C:\source\mushclient\winplace.cpp(58) : error C2065: 'GetMonitorInfo' : undeclared identifier
C:\source\mushclient\winplace.cpp(60) : error C2228: left of '.rcWork' must have class/struct/union type
Error executing cl.exe.
winplace.obj - 11 error(s), 0 warning(s)
--------------------Configuration: MUSHclient - Win32 Debug--------------------
Compiling...
winplace.cpp
C:\source\mushclient\winplace.cpp(46) : error C2065: 'HMONITOR' : undeclared identifier
C:\source\mushclient\winplace.cpp(46) : error C2146: syntax error : missing ';' before identifier 'hMonitor'
C:\source\mushclient\winplace.cpp(46) : error C2065: 'hMonitor' : undeclared identifier
C:\source\mushclient\winplace.cpp(47) : error C2065: 'MONITORINFO' : undeclared identifier
C:\source\mushclient\winplace.cpp(47) : error C2146: syntax error : missing ';' before identifier 'mi'
C:\source\mushclient\winplace.cpp(47) : error C2065: 'mi' : undeclared identifier
C:\source\mushclient\winplace.cpp(52) : error C2065: 'MonitorFromRect' : undeclared identifier
C:\source\mushclient\winplace.cpp(52) : error C2065: 'MONITOR_DEFAULTTONEAREST' : undeclared identifier
C:\source\mushclient\winplace.cpp(57) : error C2228: left of '.cbSize' must have class/struct/union type
C:\source\mushclient\winplace.cpp(58) : error C2065: 'GetMonitorInfo' : undeclared identifier
C:\source\mushclient\winplace.cpp(60) : error C2228: left of '.rcWork' must have class/struct/union type
Error executing cl.exe.
winplace.obj - 11 error(s), 0 warning(s)
...Bizzare. The MSDN docs for MonitorFromRect (for example) say to include windows.h. Maybe it's just because you're using VC6. Guess we'll have to go with the version #if...
Heh, I'm too slow around here. Seems we found the same fix (be it written a bit differently). Already sent you a pull request, not sure which version is better. Upto you. :)
//----------------------------------------
// world.BlendPixel
//----------------------------------------
static int L_BlendPixel (lua_State *L)
{
CMUSHclientDoc *pDoc = doc (L);
lua_pushnumber (L, pDoc->BlendPixel (
(long) my_checknumber (L, 1), // Blend
(long) my_checknumber (L, 2), // Base
(short) my_checknumber (L, 3), // Mode
my_optnumber (L, 4, 1) // Opacity
));
return 1; // number of result fields
} // end of L_BlendPixel
Can't you just #pragma them away? Don't you think the resulting code looks ugly? And I am a bit averse to having casts everywhere, they can hide other problems.
Before I abandoned my old branch, I had cleaned up these typecast warnings as well. You actually had macros of the form my_checklong, my_checkshort, my_optlong, my_optshort, and all they did was cast the result of my_checknumber. I used those and it looked a lot nicer, but you removed those macros later since they were unused in your repository.
[EDIT] I hate the idea of #pragma'ing away warnings, by the way. It just covers over warnings instead of fixing them, which can hide real issues down the line. If we don't care about losing data here, we should explicitly cast them.
Your reason for not having them is EXACTLY why I put them in.
The cast happens either way. Your way is implicit - you are never aware a double gets casted to a short for example, and it might bite you in the arse at some point.
With explicit casts you look at it, and are reminded 'oh, this variable is being casted to that format', and know that any data loss is intentional.
Everywhere I added a cast, I checked for the meaning of the parameter (pixel coordinate, brush, mode, flags, etc), to see if the cast was indeed supposed to happen.
There is only one cast I am not sure about: the 64-bit time cast which is the last one I added explicitly in that file, since the cast was already happening anyway - we just didn't see it.
Edit for Twisol-the-ninja:
You mentioned those on AIM. While I don't mind the existence of specific conversion functions, the implementations of merely casting was something I disagreed with. A function called my_checkshort is something I would expect to throw a lua error if the value did not fit in a short, but yours would silently cut it off. With a simple explicit (short) cast there is no such confusion about what happens in the case of an invalid value: it gets cut off.
Let me put it like this. my_checknumber returns a double (technically lua_Number which is double).
Now in my original code, the compiler automatically casts to the appropriate type for the call (and I didn't get the warning).
Now once you start putting in:
(short) my_checknumber (L, 3)
... then there is a danger you got it wrong (ie. maybe it should be a long?). In that case you have truncated a 32-bit number to a 16-bit number. Or does that generate another warning if you make a mistake? Because if not, then someone has to check every one of those to make sure you got the cast right.
Whereas, throw in a #pragma warning, you get rid of the annoying warnings, and you haven't made any silly casts.
I actually did it wrong in three cases. I had the wrong cast in my paste buffer (I first did all the longs, then all the shorts and so forth), and ended up casting some stuff that was meant to be a long into a short. The compiler yelled appropriately at me for converting it improperly.
You can see my response in commit 63ccaa27194880955902c6adc2034d1a4af9e27c on my vs2010_warnings branch.
That's the thing: they're not silly. The compiler is asking us: are you sure you want to truncate this value? The cast tells it: yes, we are sure, please truncate this value. Adding the cast also ensures the same thing happens if we were to, say, switch compilers. Do you remember that formatting bug I found that crashed when compiling under VS2005? That was because it was missing a cast, and the data-type returned from the function being used had changed.
So I am saying the original gives me no warning, and can't go wrong. Your changes could go wrong (if one of the hundreds of casts is wrong, and you got tired and made a mistake), but you have got rid of the warning.
I say it is safer to put the warning in (effectively reproducing what my version of C++ does) rather than littering the code with explicit casts.
Better to do things right the second time than wrong the first time, in my opinion. Many development teams actually insist that every warning be treated as an error. That, IMHO, is a very good thing. The compiler usually knows what it's doing, and if it sees something wrong, its usually a good idea to listen.
And that's the problem: anything meaningful the compiler has to say is drowned out by the warnings we've already acknowledged but never fixed.
Right. You are telling the compiler you know what you are doing and that it be changed to a short. For as far the compiler knows, you intend to have the value cut down to size before having it become a long again.
Is it possible you accidentally add the wrong cast? Yes. Especially in a massive operation like the one I was doing. But if you code normally and write a new function, you add those casts as you go along, and you immediately add the proper cast since you know what it is supposed to be.
If one of you insists on having a my_checkshort() function, I am fine with it. My only demand is that any such function throws a lua error if the passed value does not fit within a short, since otherwise you are just moving the problem around.
Since my project is about fixing warnings though, I am not writing that at this point - maybe when I am done, but murking up the waters with more complicated changes is what made Twisol's fixes hard to port over, right?
Worstje said: If one of you insists on having a my_checkshort() function, I am fine with it. My only demand is that any such function throws a lua error if the passed value does not fit within a short, since otherwise you are just moving the problem around.
I can't really agree with that. That changes the external contract of the methods involved, which - since it causes a Lua error - can cause plugins to break. Should they be passing anything bigger? Probably not. But this really shouldn't do anything externally visible.
Better to do things right the second time than wrong the first time, in my opinion. Many development teams actually insist that every warning be treated as an error. That, IMHO, is a very good thing. The compiler usually knows what it's doing, and if it sees something wrong, its usually a good idea to listen.
And that's the problem: anything meaningful the compiler has to say is drowned out by the warnings we've already acknowledged but never fixed.
So there is no drowning out of meaningful warnings by the compiler. I agree with you, if there was.
Twisol said:
... they're not silly. The compiler is asking us: are you sure you want to truncate this value? The cast tells it: yes, we are sure, please truncate this value.
You may be sure, and you may be wrongly sure.
By casting you are throwing away a compiler check - and to boot, possibly casting wrongly, thus throwing away precision.
Look at this:
//----------------------------------------
// world.BlendPixel
//----------------------------------------
static int L_BlendPixel (lua_State *L)
{
CMUSHclientDoc *pDoc = doc (L);
lua_pushnumber (L, pDoc->BlendPixel (
(long) my_checknumber (L, 1), // Blend
(long) my_checknumber (L, 2), // Base
(short) my_checknumber (L, 3), // Mode
my_optnumber (L, 4, 1) // Opacity
));
return 1; // number of result fields
} // end of L_BlendPixel
How can you be sure that the casts are right? You can't be, just looking at it. You need to switch documents to where BlendPixel is defined, and compare argument by argument.
By leaving the casts out, the compiler does the job right.
Look at this code:
void f (long a)
{
a = 2;
}
int main()
{
double b;
f (b);
return 0;
}
I compile that in g++ with max warnings:
$ g++ -Wall -Wextra test.cpp
$
No errors. No warnings. And that reproduces the problem - I am passing a double to a function taking a long.
Can you tell me which warning you get? I would like to look it up.
My position, subject to more information, is the compiler is wrong to give the warning. Not the code being wrong. And if there is a pragma to defeat the annoying warnings (and it only has to be used around these glue routines) that is actually the more correct solution.
Better to do things right the second time than wrong the first time, in my opinion. Many development teams actually insist that every warning be treated as an error. That, IMHO, is a very good thing. The compiler usually knows what it's doing, and if it sees something wrong, its usually a good idea to listen.
And that's the problem: anything meaningful the compiler has to say is drowned out by the warnings we've already acknowledged but never fixed.
My major point here is: I get no warnings.
We do, in newer versions of the MSVC compiler. Since VC6 was created before C++ was standardized, I'm more inclined to trust the warnings.
That said, I don't know why gcc compiles with no warnings. Strictly speaking, if you're casting from one type to another, and there is potential for data loss, a warning should be emitted, and you should explicitly cast.
Try adding the '-Wconversion' flag, as per this page: http://www.network-theory.co.uk/docs/gccintro/gccintro_31.html
Speaking of which, -Wconversion also tells you when the datatypes you're passing to a function don't match the prototype. Casting to a short when the function asks for a long will emit a warning. I think Worstje said VS2010 did that too. Don't you love compiler technology these days?
OK, adding in -Wconversion shows warnings. But this is cranking up the warnings pretty high.
If I change my project settings to go from level 3 warnings to level 4 warnings I get 771 warnings, quite a few of which are in zlib library, PCRE library, luacom, SQLite3 and even the Visual Studio include files.
So once the compiler-vendor supplied files themselves generate warnings, I decided to run with level 3 warnings. And I compile with zero warnings at that level, and admittedly pragma out a couple (mainly in the external libraries).
I think you can go overboard a bit with warnings. It's like the warning on the tractor (Warning: avoid death) or the iron (Warning: gets hot) or the knife (Warning: edge is sharp).
Yes, OK. My point is that if level 3 warnings are OK for Zlib, PCRE, LuaCOM, SQLite3 - they are OK for me too.
Especially when to defeat the warnings you potentially make an incorrect typecast.
However having said all that, I am fixing some stuff I spotted with the warnings cranked up to level 4, then I'll put it back again. Or maybe I can pragma the less egregious ones away and leave it at level 4.
Well, I think -Wconversion can be set separately from the normal levels? I'm not entirely sure though.
We're working from warning level 3, too. 4 definitely has a lot of perhaps unrelated stuff, even frivolous. But there are still a lot of good ones at level 3, and IIRC I still got the majority at level 2.
One more thing, which I do not think has been mentioned here... VC++ 6 is horribly out-of-date, and it even was quite a bit deviating from the C++ standard when it came out if I read the sources on the internet I came across while working on the numerous warnings.
Newer versions of MSVC++ are aiming to be more compliant, and the new warnings are a natural extension of that. So, in my eyes, even if in VC++6 it compiles without warnings, it is very much worth it to (properly) fix the warnings seen in higher versions.
Of course, properly fixing and alleviating the symptoms by making the warnings go away aren't always the same thing. As such I only touch things I fully understand, and (in the example of the typecasts) do not add them willy-nilly, but try to understand if I can fix the issue by changing the types of a few parameters so instead of five typecasts to one function, only the function holds the one typecast.
Either way, peer review is as always needed, since we be humans and we make booboos.
Commit a0d2cd3994a has had quite a bit of work on the warnings. First I bumped the warning level up to 4 (from 3) which is the highest level I can use.
Then I (admittedly) added in some pragmas to reduce the warnings back to manageable levels. Each pragma is now documented so you can see exactly what is being suppressed.
In the cases where I thought the warnings had a point the code was revamped to get rid of the problem (eg. unused variables, uninitialized variables).
You are most welcome to review my changes and check I didn't accidentally go too far, or change the actual code behaviour.
Ignoring the warnings I disabled in the solution regarding deprecated/safe functions, your current master gives me 525 warnings still. In comparison, using my own vs2010_warnings branch, I am down to 144 warnings.
Can you do some cross-checking into my changes and merging what seems useful? I'll definitely be looking through your changes as well.
That said - I still will voice my unhappiness about pragma-hiding LVL3/2/1 warnings. Warnings are given with good reasons and to hide them is an ostrich mentality imo.
Well then you better fire off an email to the zlib maintainers for a start:
c:\source\mushclient\zlib\inflate.c(618) : warning C4127: conditional expression is constant
c:\source\mushclient\zlib\inflate.c(629) : warning C4127: conditional expression is constant
c:\source\mushclient\zlib\inflate.c(629) : warning C4127: conditional expression is constant
c:\source\mushclient\zlib\inflate.c(633) : warning C4127: conditional expression is constant
c:\source\mushclient\zlib\inflate.c(634) : warning C4127: conditional expression is constant
c:\source\mushclient\zlib\inflate.c(655) : warning C4127: conditional expression is constant
c:\source\mushclient\zlib\inflate.c(668) : warning C4127: conditional expression is constant
c:\source\mushclient\zlib\inflate.c(672) : warning C4127: conditional expression is constant
c:\source\mushclient\zlib\inflate.c(672) : warning C4127: conditional expression is constant
c:\source\mushclient\zlib\inflate.c(686) : warning C4127: conditional expression is constant
c:\source\mushclient\zlib\inflate.c(687) : warning C4127: conditional expression is constant
c:\source\mushclient\zlib\inflate.c(690) : warning C4127: conditional expression is constant
c:\source\mushclient\zlib\inflate.c(690) : warning C4127: conditional expression is constant
c:\source\mushclient\zlib\inflate.c(693) : warning C4127: conditional expression is constant
c:\source\mushclient\zlib\inflate.c(694) : warning C4127: conditional expression is constant
c:\source\mushclient\zlib\inflate.c(697) : warning C4127: conditional expression is constant
c:\source\mushclient\zlib\inflate.c(697) : warning C4127: conditional expression is constant
c:\source\mushclient\zlib\inflate.c(702) : warning C4127: conditional expression is constant
...
c:\source\mushclient\zlib\trees.c(595) : warning C4244: '=' : conversion from 'int ' to 'unsigned short ', possible loss of data
c:\source\mushclient\zlib\trees.c(608) : warning C4244: '=' : conversion from 'unsigned int ' to 'unsigned short ', possible loss of data
c:\source\mushclient\zlib\trees.c(624) : warning C4131: 'build_tree' : uses old-style declarator
c:\source\mushclient\zlib\trees.c(680) : warning C4244: '=' : conversion from 'int ' to 'unsigned short ', possible loss of data
c:\source\mushclient\zlib\trees.c(712) : warning C4131: 'scan_tree' : uses old-style declarator
c:\source\mushclient\zlib\trees.c(732) : warning C4244: '+=' : conversion from 'int ' to 'unsigned short ', possible loss of data
c:\source\mushclient\zlib\trees.c(757) : warning C4131: 'send_tree' : uses old-style declarator
c:\source\mushclient\zlib\trees.c(777) : warning C4244: '=' : conversion from 'int ' to 'unsigned short ', possible loss of data
c:\source\mushclient\zlib\trees.c(781) : warning C4244: '=' : conversion from 'int ' to 'unsigned short ', possible loss of data
c:\source\mushclient\zlib\trees.c(784) : warning C4244: '=' : conversion from 'int ' to 'unsigned short ', possible loss of data
Then send one to the PNG guys:
c:\source\mushclient\png\pngrtran.c(3701) : warning C4244: 'initializing' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\png\pngrtran.c(3702) : warning C4244: 'initializing' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\png\pngrtran.c(3733) : warning C4244: 'initializing' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\png\pngrtran.c(3734) : warning C4244: 'initializing' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\png\pngrtran.c(3735) : warning C4244: 'initializing' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\png\pngrtran.c(3751) : warning C4244: 'initializing' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\png\pngrtran.c(3752) : warning C4244: 'initializing' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\png\pngrtran.c(3753) : warning C4244: 'initializing' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\png\pngrtran.c(3754) : warning C4244: 'initializing' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\png\pngrtran.c(3755) : warning C4244: 'initializing' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\png\pngrtran.c(3756) : warning C4244: 'initializing' : conversion from 'int ' to 'unsigned char ', possible loss of data
Then write to the SQLite3 maintainers:
c:\source\mushclient\sqlite3\sqlite3.c(19506) : warning C4127: conditional expression is constant
c:\source\mushclient\sqlite3\sqlite3.c(19508) : warning C4127: conditional expression is constant
c:\source\mushclient\sqlite3\sqlite3.c(19513) : warning C4127: conditional expression is constant
c:\source\mushclient\sqlite3\sqlite3.c(36797) : warning C4127: conditional expression is constant
c:\source\mushclient\sqlite3\sqlite3.c(39678) : warning C4127: conditional expression is constant
c:\source\mushclient\sqlite3\sqlite3.c(39695) : warning C4127: conditional expression is constant
c:\source\mushclient\sqlite3\sqlite3.c(46402) : warning C4127: conditional expression is constant
c:\source\mushclient\sqlite3\sqlite3.c(46495) : warning C4127: conditional expression is constant
c:\source\mushclient\sqlite3\sqlite3.c(50414) : warning C4127: conditional expression is constant
c:\source\mushclient\sqlite3\sqlite3.c(55236) : warning C4132: 'dummy' : const object should be initialized
c:\source\mushclient\sqlite3\sqlite3.c(68704) : warning C4127: conditional expression is constant
c:\source\mushclient\sqlite3\sqlite3.c(74335) : warning C4127: conditional expression is constant
c:\source\mushclient\sqlite3\sqlite3.c(74472) : warning C4127: conditional expression is constant
Then the guys that do LuaCOM:
c:\source\mushclient\luacom\tluacontrol.h(184) : warning C4189: 'hr' : local variable is initialized but not referenced
c:\source\mushclient\luacom\luacom.cpp(578) : warning C4189: 'pCF' : local variable is initialized but not referenced
c:\source\mushclient\luacom\luacom.cpp(787) : warning C4189: 'cpc' : local variable is initialized but not referenced
c:\source\mushclient\luacom\luacom.cpp(1900) : warning C4189: 'hr' : local variable is initialized but not referenced
c:\source\mushclient\luacom\luacom.cpp(1901) : warning C4189: 'pfuncdesc' : local variable is initialized but not referenced
c:\source\mushclient\luacom\luacom.cpp(1999) : warning C4189: 'hr' : local variable is initialized but not referenced
c:\source\mushclient\luacom\luacom.cpp(2015) : warning C4189: 'self_param' : local variable is initialized but not referenced
c:\source\mushclient\luacom\luacom.cpp(2072) : warning C4189: 'hr' : local variable is initialized but not referenced
c:\source\mushclient\luacom\luacom.cpp(2079) : warning C4189: 'index_param' : local variable is initialized but not referenced
c:\source\mushclient\luacom\luacom.cpp(2172) : warning C4189: 'hr' : local variable is initialized but not referenced
And the guy that does PCRE:
c:\source\mushclient\pcre\pcre_compile.c(948) : warning C4244: '=' : conversion from 'int ' to 'char ', possible loss of data
c:\source\mushclient\pcre\pcre_compile.c(958) : warning C4244: '=' : conversion from 'int ' to 'char ', possible loss of data
c:\source\mushclient\pcre\pcre_compile.c(2294) : warning C4244: '=' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\pcre\pcre_compile.c(2294) : warning C4244: '=' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\pcre\pcre_compile.c(2305) : warning C4244: '=' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\pcre\pcre_compile.c(2305) : warning C4244: '=' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\pcre\pcre_compile.c(2334) : warning C4244: '=' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\pcre\pcre_compile.c(2334) : warning C4244: '=' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\pcre\pcre_compile.c(2361) : warning C4244: '=' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\pcre\pcre_compile.c(2361) : warning C4244: '=' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\pcre\pcre_compile.c(3190) : warning C4244: '=' : conversion from 'int ' to 'unsigned char ', possible loss of data
c:\source\mushclient\pcre\pcre_compile.c(3262) : warning C4244: '=' : conversion from 'int ' to 'unsigned char ', possible loss of data
Unless you can convince all of them to "fix" their code (and they might conceivably debate the point with you) then I have to, ostrich-like, use some pragmas.
My main point here is, if it is OK for all these well-known libraries (with a far greater audience than MUSHclient) to accept assignments that, technically, might involve loss of data (and which may never happen because they know the sort of data they have) then I don't see why I should clutter up my code with hundreds of ugly type-casts, to reach a higher standard then they are.
And this is apart from my earlier objection that type-casts are, themselves, not particularly ideal.
I get none of those warnings using a clean build with /W3 straight from your master. Did you do anything special?
If you are talking W4 warnings now, well... I'll have to point out I was talking about levels 1-3. Level 4 does get pedantic, but below that the warnings have good reasons for being there in my opinion.
Nick Gammon said: Unless you can convince all of them to "fix" their code (and they might conceivably debate the point with you) then I have to, ostrich-like, use some pragmas.
Or split these third-party libraries into their own projects, or even compile them entirely separately and put the resulting libs in a folder in the repo. External libraries are not our responsibility unless we want to modify them. The reasonable thing to do is isolate them from MUSHclient's source, IMHO. It's not hard, I did it quite easily in my 'old' branch.
And you're right, type casts aren't ideal. C++ created (static|dynamic|const|reinterpret)_cast to make the operation highly explicit, and the ugliness was intentional, to make you think twice about what you're doing. The solution isn't to not explicitly cast, it's to determine whether you can write it so it doesn't need a cast. And of course, quite often you can't... so the casts make it blatantly obvious that you're truncating the data, turning it into a derived class pointer, removing const-ness, or reinterpreting it outright.
I get none of those warnings using a clean build with /W3 straight from your master. Did you do anything special?
Level 4 warnings. Although I had some pragmas in before today to suppress a few warnings at level 3.
The thing is, if you choose to use /W3 you are already choosing to ignore some things (warnings). And if Microsoft moved some warnings from level 4 to level 3 (thus triggering some of the things you are seeing but I don't) then it surely is a matter of opinion, rather than fact, that is is OK to ignore this warning, but not that warning.
I mean, unless you get rid of every warning at level 4, you are basically saying "I am (sensibly) ignoring some warnings, but Nick is (ostrich-like) ignoring other ones".
Ok, so apparently git does not like to pull everything sometimes. I pulled yet again, and this time it seems I got all your warning pragma's properly.
Still have 7 warnings, some of which I fixed in my branch.
Another I can't be bummed to fix in git but should be easy to do... in number.c, move the number.h include to be below the stdlib.h include. The latter includes limits.h, causing LONG_MAX to be defined twice.
Edit: You ninja! :)
That's a harsh way to put it, but maybe I set myself up for that comment (some sayings don't pass through the language barrier unscathed it seems). My point however is that in the things I have seen/heard regarding warning levels in C++ development, /W3 is a good sweet spot. I could point to [1] and say 'see?', but it is a vague page. My interpretation of it is 'W4 is pedantic, W2 is for serious booboos, and W3 is on the edge of becoming a serious booboo.'. Thus, I feel 'sensible' in picking W3 over W4.
If you want, I'll gladly throw myself at W4 warnings too. But with the amount of warnings there... W3 has enough (hidden) warnings to scare the bejeesus out of a nightmare. One thing at a time seems sensible to me?
I'm glad you are down to 7 warnings, that sounds much more manageable. I incorporated most of them in the latest commit.
I refrained on the typecast as I believe that discussion is still ongoing.
You may see in the commit history I accidentally pulled in more than I wanted of yours, so I had to revert it out. Looks like I have to learn more about Git. :)
#pragma warning (disable : 4800) // forcing value to bool 'true' or 'false' (performance warning)
I don't have the code in front of me right now, but if I understand correctly, the issue is assigning a numeric value to a bool variable? I believe I had removed that in my old branch altogether, and replaced 'b = i' with 'b = (i != 0)', which to my mind is more explicit anyways.
It really bothers me that we're modifying the external libraries just to appease the warnings. *shudder* I still believe that external libraries are not something we should worry about, and the proper way to avoid seeing the warnings is to compile them as separate projects altogether.
Still working through the batch of changes, but it looks good so far. :)
[EDIT]: Ooh, nice catch in lua_scripting.cpp. We'd never have seen this normally:
// global table will not have _G prefix
- if (sTableName != "_G")
+ if (strcmp (sTableName, "_G") != 0)
Nick, your second revert reverted pretty much the entirety of Worstje's fixes branch. Just checking, was that intended? If you look at the commit changes for that revert, it's massive [1]. Using Compare View, there are only three files that have been changed, and that was from the last commit [2].
#pragma warning (disable : 4800) // forcing value to bool 'true' or 'false' (performance warning)
I don't have the code in front of me right now, but if I understand correctly, the issue is assigning a numeric value to a bool variable? I believe I had removed that in my old branch altogether, and replaced 'b = i' with 'b = (i != 0)', which to my mind is more explicit anyways.
It can be reproduced like this:
BOOL a = TRUE;
bool b = a; // forcing value to bool 'true' or 'false' (performance warning)
Now given a BOOL (say, an argument to a function) I just can't come at doing this:
Please don't forget my fix to ProcessPreviousLine.cpp at line 749. It has one of the surviving warnings still. (Not sure if you're getting that one on VC++6, so I figured I'd mention it.)