[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,229 posts)  [Biography] bio
Date Reply #15 on Thu 16 Sep 2010 01:54 AM (UTC)  [ quote  ]

Amended on Thu 16 Sep 2010 01:56 AM (UTC) by Twisol

Message
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.
'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 #16 on Thu 16 Sep 2010 01:58 AM (UTC)  [ quote  ]

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

Message
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.
[Go to top] top
Posted by Nick Gammon   Australia  (18,770 posts)  [Biography] bio   Forum Administrator
Date Reply #17 on Thu 16 Sep 2010 01:59 AM (UTC)  [ quote  ]
Message
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.
- 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 Reply #18 on Thu 16 Sep 2010 02:02 AM (UTC)  [ quote  ]
Message
Example:


//----------------------------------------
//  world.BlendPixel
//----------------------------------------
static int L_BlendPixel (lua_State *L)
  {
  CMUSHclientDoc *pDoc = doc (L);
  lua_pushnumber (L, pDoc->BlendPixel (
            (short) my_checknumber (L, 1),    // Blend
            my_checknumber (L, 2),    // Base
            my_checknumber (L, 3),    // Mode
            my_optnumber (L, 4, 1)    // Opacity
            ));
  return 1;  // number of result fields
  } // end of L_BlendPixel


The cast to short is incorrect, but I get no warning.
- Nick Gammon

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

Amended on Thu 16 Sep 2010 02:04 AM (UTC) by Worstje

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

Edit: Weird, now I am going to doublecheck.
[Go to top] top
Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Reply #20 on Thu 16 Sep 2010 02:05 AM (UTC)  [ quote  ]
Message
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.
'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 Reply #21 on Thu 16 Sep 2010 02:06 AM (UTC)  [ quote  ]
Message
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.
- Nick Gammon

www.gammon.com.au, www.mushclient.com
[Go to top] top
Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Reply #22 on Thu 16 Sep 2010 02:08 AM (UTC)  [ quote  ]
Message
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.
'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 #23 on Thu 16 Sep 2010 02:09 AM (UTC)  [ quote  ]

Amended on Thu 16 Sep 2010 02:12 AM (UTC) by Worstje

Message
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?
[Go to top] top
Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Reply #24 on Thu 16 Sep 2010 02:16 AM (UTC)  [ quote  ]
Message
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.
'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 Reply #25 on Thu 16 Sep 2010 02:30 AM (UTC)  [ quote  ]
Message
Twisol said:

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.

Look:


--------------------Configuration: MUSHclient - Win32 Debug--------------------

MUSHclient.exe - 0 error(s), 0 warning(s)


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.
- 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 Reply #26 on Thu 16 Sep 2010 02:35 AM (UTC)  [ quote  ]

Amended on Thu 16 Sep 2010 02:36 AM (UTC) by Nick Gammon

Message
Twisol said:

If we don't care about losing data here, we should explicitly cast them.


I care about losing data which is why I don't cast them. By which I mean, if you cast what should be a long to a short, you have lost data.
- Nick Gammon

www.gammon.com.au, www.mushclient.com
[Go to top] top
Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Reply #27 on Thu 16 Sep 2010 02:38 AM (UTC)  [ quote  ]
Message
Nick Gammon said:

Twisol said:

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

It says it's not included in -Wall.
'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 Reply #28 on Thu 16 Sep 2010 02:41 AM (UTC)  [ quote  ]
Message
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?
'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 Reply #29 on Thu 16 Sep 2010 03:20 AM (UTC)  [ quote  ]

Amended on Thu 16 Sep 2010 03:21 AM (UTC) by Nick Gammon

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

[EDIT] Actually after doing "rebuild all" I get:


MUSHclient.exe - 0 error(s), 5160 warning(s)


So that's quite a job, eh?, to fix all that.
- Nick Gammon

www.gammon.com.au, www.mushclient.com
[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,912 views.

This is page 2, 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]