[Home] [Downloads] [Search] [Help/forum]

Gammon Software Solutions forum

See www.mushclient.com/spam for dealing with forum spam. Please read the MUSHclient FAQ!

[Folder]  Entire forum
-> [Folder]  MUSHclient
. -> [Folder]  Development
. . -> [Subject]  Fixing up warnings so others can better contribute
Home  |  Users  |  Search  |  FAQ
Username:
Register forum user name
Password:
Forgotten password?
Fixing up warnings so others can better contribute

[Reply to this subject]  Reply to this subject   [New subject]  Start a new subject   [Refresh] Refresh page

Pages: 1  2  3 4  

Posted by Twisol   USA  (2,230 posts)  [Biography] bio
Date Reply #30 on Thu 16 Sep 2010 03:23 AM (UTC)  [ quote  ]
Message
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.
'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol
[Go to top] top
Posted by Worstje   Netherlands  (867 posts)  [Biography] bio
Date Reply #31 on Thu 16 Sep 2010 05:16 AM (UTC)  [ quote  ]
Message
I am working with /W3 myself.

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.
[Go to top] top
Posted by Twisol   USA  (2,230 posts)  [Biography] bio
Date Reply #32 on Thu 16 Sep 2010 05:18 AM (UTC)  [ quote  ]
Message
Yep, it's been mentioned, though not in detail:
Twisol said:

Nick Gammon said:
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.
'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol
[Go to top] top
Posted by Nick Gammon   Australia  (18,797 posts)  [Biography] bio   Forum Administrator
Date Reply #33 on Thu 16 Sep 2010 06:00 AM (UTC)  [ quote  ]

Amended on Thu 16 Sep 2010 06:01 AM (UTC) by Nick Gammon

Message
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.

http://github.com/nickgammon/mushclient/commit/a0d2cd3994a
- Nick Gammon

www.gammon.com.au, www.mushclient.com
[Go to top] top
Posted by Worstje   Netherlands  (867 posts)  [Biography] bio
Date Reply #34 on Thu 16 Sep 2010 06:13 AM (UTC)  [ quote  ]
Message
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.
[Go to top] top
Posted by Nick Gammon   Australia  (18,797 posts)  [Biography] bio   Forum Administrator
Date Reply #35 on Thu 16 Sep 2010 06:21 AM (UTC)  [ quote  ]
Message
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.
- Nick Gammon

www.gammon.com.au, www.mushclient.com
[Go to top] top
Posted by Nick Gammon   Australia  (18,797 posts)  [Biography] bio   Forum Administrator
Date Reply #36 on Thu 16 Sep 2010 06:25 AM (UTC)  [ quote  ]
Message
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.
- Nick Gammon

www.gammon.com.au, www.mushclient.com
[Go to top] top
Posted by Worstje   Netherlands  (867 posts)  [Biography] bio
Date Reply #37 on Thu 16 Sep 2010 06:33 AM (UTC)  [ quote  ]
Message
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.
[Go to top] top
Posted by Twisol   USA  (2,230 posts)  [Biography] bio
Date Reply #38 on Thu 16 Sep 2010 06:35 AM (UTC)  [ quote  ]
Message
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.
'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol
[Go to top] top
Posted by Nick Gammon   Australia  (18,797 posts)  [Biography] bio   Forum Administrator
Date Reply #39 on Thu 16 Sep 2010 06:53 AM (UTC)  [ quote  ]
Message
Worstje said:

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".
- Nick Gammon

www.gammon.com.au, www.mushclient.com
[Go to top] top
Posted by Worstje   Netherlands  (867 posts)  [Biography] bio
Date Reply #40 on Thu 16 Sep 2010 06:53 AM (UTC)  [ quote  ]

Amended on Thu 16 Sep 2010 07:08 AM (UTC) by Worstje

Message
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?

[1] http://msdn.microsoft.com/en-us/library/ms937402.aspx
[Go to top] top
Posted by Nick Gammon   Australia  (18,797 posts)  [Biography] bio   Forum Administrator
Date Reply #41 on Thu 16 Sep 2010 07:20 AM (UTC)  [ quote  ]
Message
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. :)
- Nick Gammon

www.gammon.com.au, www.mushclient.com
[Go to top] top
Posted by Twisol   USA  (2,230 posts)  [Biography] bio
Date Reply #42 on Thu 16 Sep 2010 07:28 AM (UTC)  [ quote  ]
Message
Just as a small note, but you have this:
#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.
'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol
[Go to top] top
Posted by Twisol   USA  (2,230 posts)  [Biography] bio
Date Reply #43 on Thu 16 Sep 2010 07:30 AM (UTC)  [ quote  ]
Message 
+#pragma warning( disable : 4100)  // unreferenced formal parameter

Easily suppressed in my experience by removing the name of the variable from the list:

void foobar (int one, int, int three)
{
  return one + three;
}


But I can't test that right now unfortunately.
'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol
[Go to top] top
Posted by Twisol   USA  (2,230 posts)  [Biography] bio
Date Reply #44 on Thu 16 Sep 2010 07:34 AM (UTC)  [ quote  ]

Amended on Thu 16 Sep 2010 07:36 AM (UTC) by Twisol

Message
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)
'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol
[Go to top] top

The dates and times for posts above are shown in Universal Co-ordinated Time (UTC).

To show them in your local time you can join the forum, and then set the 'time correction' field in your profile to the number of hours difference between your location and UTC time.

8,192 views.

This is page 3, subject is 4 pages long:  [Previous page]  1  2  3 4  [Next page]

[Reply to this subject]  Reply to this subject   [New subject]  Start a new subject   [Refresh] Refresh page

Go to topic:           Search the forum

[Go to top] top

[Home]

Written by Nick Gammon - 5K

Comments to: Gammon Software support
[RH click to get RSS URL] Forum RSS feed ( http://www.gammon.com.au/rss/forum.xml )

[Best viewed with any browser - 2K]    [Internet Contents Rating Association (ICRA) - 2K]    [Web site powered by FutureQuest.Net]