[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?
(New message)
Subject: Fixing up warnings so others can better contribute
Name:
Your forum user name.
Register forum user name
Password:
Your forum password.
Forgotten password?
Message:
Message to be posted (in English, please).
Forum codes:
Check this if your message uses 'forum codes' or templates (auto-detected for new posts).
Forum codes Templates

Save this message ...

Subject review (reverse sequence)

Pages: 1  2  3 4  

Posted by Nick Gammon   Australia  (18,770 posts)  [Biography] bio   Forum Administrator
Date Thu 16 Sep 2010 09:19 AM (UTC)  [ quote  ]
Message
Got it now, thanks.
- Nick Gammon

www.gammon.com.au, www.mushclient.com
[Go to top] top
Posted by Worstje   Netherlands  (867 posts)  [Biography] bio
Date Thu 16 Sep 2010 07:52 AM (UTC)  [ quote  ]
Message
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.)
[Go to top] top
Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Thu 16 Sep 2010 07:48 AM (UTC)  [ quote  ]
Message
Nick Gammon said:
  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:
bool b = a != 0;

It's just ridiculous.

It would be like saying in Lua:
if a ~= false and a ~= nil then ...

Instead of:
if a then ...

BOOL is a typedef for some kind of int type, since win32 was created when C had no explicit bool. The Lua analog, I suppose, would be:
if a and a ~= 0 then ... end
'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol
[Go to top] top
Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Thu 16 Sep 2010 07:45 AM (UTC)  [ quote  ]
Message
Ah. `git cherry-pick` is the way to go, I suppose.
'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol
[Go to top] top
Posted by Nick Gammon   Australia  (18,770 posts)  [Biography] bio   Forum Administrator
Date Thu 16 Sep 2010 07:44 AM (UTC)  [ quote  ]
Message
Twisol said:

Nick, your second revert reverted pretty much the entirety of Worstje's fixes branch. Just checking, was that intended?


Yes, I was trying to pull in just the one commit that he mentioned fixes a few of the last 7 warnings. Not the huge numbers of typecasts he did.
- Nick Gammon

www.gammon.com.au, www.mushclient.com
[Go to top] top
Posted by Nick Gammon   Australia  (18,770 posts)  [Biography] bio   Forum Administrator
Date Thu 16 Sep 2010 07:43 AM (UTC)  [ quote  ]
Message
Twisol said:

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.


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:


bool b = a != 0;


It's just ridiculous.

It would be like saying in Lua:


if a ~= false and a ~= nil then ...


Instead of:


if a then ...

- Nick Gammon

www.gammon.com.au, www.mushclient.com
[Go to top] top
Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Thu 16 Sep 2010 07:41 AM (UTC)  [ quote  ]

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

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

[1] http://github.com/nickgammon/mushclient/commit/07792acff259be0b0c114e504b059be40f3707db

[2] http://github.com/nickgammon/mushclient/compare/5f711c8853...e2ecc40e85
'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol
[Go to top] top
Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date 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
Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date 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,229 posts)  [Biography] bio
Date 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 Nick Gammon   Australia  (18,770 posts)  [Biography] bio   Forum Administrator
Date 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 Worstje   Netherlands  (867 posts)  [Biography] bio
Date 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,770 posts)  [Biography] bio   Forum Administrator
Date 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 Twisol   USA  (2,229 posts)  [Biography] bio
Date 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 Worstje   Netherlands  (867 posts)  [Biography] bio
Date 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

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.

7,946 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]