Register forum user name Search FAQ

Gammon Forum

Notice: Any messages purporting to come from this site telling you that your password has expired, or that you need to verify your details, confirm your email, resolve issues, making threats, or asking for money, are spam. We do not email users with any such messages. If you have lost your password you can obtain a new one by using the password reset link.

Due to spam on this forum, all posts now need moderator approval.

 Entire forum ➜ MUSHclient ➜ Development ➜ Improvements to plugin callbacks
Improvements to plugin callbacks

It is now over 60 days since the last post. This thread is closed.     Refresh page

Pages: 1  2 3  

Posted by Twisol   USA  (2,257 posts)  Bio
Date Reply #15 on Sat 18 Sep 2010 05:52 AM (UTC)

Amended on Sat 18 Sep 2010 05:55 AM (UTC) by Twisol

Message
Wow - this looks pretty bad:

#pragma warning (disable : 4018)  // '<' : signed/unsigned mismatch
#pragma warning (disable : 4100)  // unreferenced formal parameter
#pragma warning (disable : 4127)  // conditional expression is constant
#pragma warning (disable : 4201)  // nonstandard extension used : nameless struct/union
#pragma warning (disable : 4244)  // conversion from 'int ' to 'char ', possible loss of data
#pragma warning (disable : 4244)  // conversion from 'int' to 'unsigned short', possible loss of data
#pragma warning (disable : 4503)  // decorated name length exceeded, name was truncated
#pragma warning (disable : 4505)  // unreferenced local function has been removed
#pragma warning (disable : 4511)  // copy constructor could not be generated
#pragma warning (disable : 4512)  // assignment operator could not be generated
#pragma warning (disable : 4514)  // unreferenced inline function has been removed
#pragma warning (disable : 4611)  // interaction between '_setjmp' and C++ object destruction is non-portable
#pragma warning (disable : 4663)  // C++ language change: to explicitly specialize class template yadda yadda
#pragma warning (disable : 4702)  // unreachable code
#pragma warning (disable : 4706)  // assignment within conditional expression
#pragma warning (disable : 4710)  // function 'x' not inlined
#pragma warning (disable : 4786)  // identifier was truncated to 'number' characters in the debug information

That's in stdafx.h. Those warnings are disabled globally.

[EDIT]: The bolded one frightens me badly. Lua uses setjmp and longjmp for error handling. If an error occurs, destructors in the part of the stack that's jumped out of are never called. This is exactly what I mentioned a day or so ago, and since it's disabled globally, we have no idea at this point if it's happening already.
'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol
Top
Posted by Nick Gammon   Australia  (23,158 posts)  Bio   Forum Administrator
Date Reply #16 on Sat 18 Sep 2010 05:56 AM (UTC)
Message
Well, I disabled this warning for the SQLite3 code:


//# pragma warning (disable : 4244)  // conversion from 'int ' to 'char ', possible loss of data


Then I compiled and got 143 warnings. Some at least are ridiculous IMHO. For example:


  p->validTZ = (p->tz!=0)?1:0;


So that code assigns 1 or 0 to a field p->validTZ. And how is that defined?


  char validJD;      /* True (1) if iJD is valid */


I call that a spurious warning. So I'm not going to put in:


  p->validTZ = (char) (p->tz!=0)?1:0;


There are other cases where you get a 'signed/unsigned' mismatch. But in cases where you know that the code will not exceed the sign boundary (eg. i = 1 to 10) then the warning is ridiculous. The thing to focus on is that fixing all warnings will not get rid of all your bugs. For example:


char * p = NULL;

*p = 'a';


No warning there, but it crashes. So you have to look at your code and understand what it is doing.

Certainly warnings are helpful, and as I said, I always compile to zero warnings (previously at level 3, and now at level 4) - subject to suppressing a few of course.

Worstje already said he doesn't work to level 4, so there is an implicit assertion there that some warnings are OK (or should I say, OK to suppress).

You could conceivably go through the code and get rid of all the type-conversion warnings, but this is on a similar vein to the refactoring for niceness rather than any explicit bug report or problem. For that matter you could go through and make it more Unicode-aware.

Let's focus on the changes to the plugin handling. I think I'm on top of it now, so if you want to test and see if it runs your plugins the way you expect, please do.


- Nick Gammon

www.gammon.com.au, www.mushclient.com
Top
Posted by Twisol   USA  (2,257 posts)  Bio
Date Reply #17 on Sat 18 Sep 2010 06:02 AM (UTC)
Message
Nick Gammon said:
Well, I disabled this warning for the SQLite3 code:

Is that external-library code? I don't believe external libraries should be our problem. If we want to avoid seeing those warnings, we can compile them as separate projects under the same solution or something.

Nick Gammon said:
There are other cases where you get a 'signed/unsigned' mismatch. But in cases where you know that the code will not exceed the sign boundary (eg. i = 1 to 10) then the warning is ridiculous.

In those cases, I think you can just define i as an unsigned int, can't you? It's only frivolous because we as humans can tell it's not a problem.

Nick Gammon said:
The thing to focus on is that fixing all warnings will not get rid of all your bugs.

[...]

No warning there, but it crashes. So you have to look at your code and understand what it is doing.

No, but it sure can help find some of them, and prevent others. IIRC, a few were fixed in the recent batches of warning cleanups. Sure, it's not a panacea, but then nothing is. Yet an inch of prevention helps safeguard for the future.

Nick Gammon said:
Let's focus on the changes to the plugin handling. I think I'm on top of it now, so if you want to test and see if it runs your plugins the way you expect, please do.

Alrighty.
'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol
Top
Posted by Nick Gammon   Australia  (23,158 posts)  Bio   Forum Administrator
Date Reply #18 on Sat 18 Sep 2010 06:10 AM (UTC)

Amended on Sat 18 Sep 2010 06:11 AM (UTC) by Nick Gammon

Message
Twisol said:

Wow - this looks pretty bad:

That's in stdafx.h. Those warnings are disabled globally.


Well we accept the warnings or we don't, eh?

Twisol said:

: The bolded one frightens me badly. Lua uses setjmp and longjmp for error handling. If an error occurs, destructors in the part of the stack that's jumped out of are never called. This is exactly what I mentioned a day or so ago, and since it's disabled globally, we have no idea at this point if it's happening already.


Don't be too frightened. The code is part of the PNG stuff and is their recommended way of doing it. It only gets exercised when reading PNG files, and so far nothing has happened.

This is the meaning BTW:



Compiler Warning (level 4) C4611

interaction between '_setjmp' and C++ object destruction is non-portable

On some platforms, functions that include _setjmp may not support C++ object semantics of destruction when out of scope.

To avoid unexpected behavior, avoid using _setjmp in functions that have constructors and destructors.



I don't know how to interpret that "on some platforms". Do they mean Unix? Or some versions of Windows? Or what?

If the recommended way of reading PNG files doesn't work on Windows, then there is a problem. Perhaps the reading could be hived off to a function that is not a class.

And I don't know if the code in question exercises any destructors. It is all basically in a single function. I mean if you can demonstrate in what way the problem case would arise, and indeed if it applies to this platform, then the code can be reworked. But let's not get too excited.

I have moved that particular warning to the file(s) in question to localize it a bit.
- Nick Gammon

www.gammon.com.au, www.mushclient.com
Top
Posted by Nick Gammon   Australia  (23,158 posts)  Bio   Forum Administrator
Date Reply #19 on Sat 18 Sep 2010 06:15 AM (UTC)
Message
Twisol said:

If we want to avoid seeing those warnings, we can compile them as separate projects under the same solution or something.


If we are unworried enough to do "something" like compiling them separately and then forgetting the warnings happened, we may as well pragma them away.
- Nick Gammon

www.gammon.com.au, www.mushclient.com
Top
Posted by Twisol   USA  (2,257 posts)  Bio
Date Reply #20 on Sat 18 Sep 2010 06:18 AM (UTC)
Message
Nick Gammon said:
Well we accept the warnings or we don't, eh?

Eh, it comes down to the context-sensitivity thing. Disabling a warning globally like that gives me the heebie-jeebies. I like things to be very explicit; if something's unclear or I'm missing a key piece of information, it's more likely I'll mess it up.

I found a nice StackOverflow topic on warnings, by the way:
http://stackoverflow.com/questions/183788/c-c-compiler-warnings-do-you-clean-up-all-your-code-to-remove-them-or-leave

Nick Gammon said:
Don't be too frightened. The code is part of the PNG stuff and is their recommended way of doing it. It only gets exercised when reading PNG files, and so far nothing has happened.

If it's specific to PNG stuff, it shouldn't be global to all of MUSHclient. Thank you for moving it!

Nick Gammon said:
This is the meaning BTW:

-----

Compiler Warning (level 4) C4611

interaction between '_setjmp' and C++ object destruction is non-portable

On some platforms, functions that include _setjmp may not support C++ object semantics of destruction when out of scope.

To avoid unexpected behavior, avoid using _setjmp in functions that have constructors and destructors.

-----

I don't know how to interpret that "on some platforms". Do they mean Unix? Or some versions of Windows? Or what?

Well, there's this Lua mailing list post:
http://lua-users.org/wiki/ErrorHandlingBetweenLuaAndCplusplus

Basically, setjmp is not guaranteed to call destructors. If it does, it's because your compiler is keeping you safe, not because the standard says so. setjmp is part of C, and C has no destructors.

Nick Gammon said:
If the recommended way of reading PNG files doesn't work on Windows, then there is a problem. Perhaps the reading could be hived off to a function that is not a class.

And I don't know if the code in question exercises any destructors. It is all basically in a single function. I mean if you can demonstrate in what way the problem case would arise, and indeed if it applies to this platform, then the code can be reworked. But let's not get too excited.

I think PNG is written in C, isn't it? It should be fine - which brings me back to my "third-party libraries are not our business" position.
'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol
Top
Posted by Twisol   USA  (2,257 posts)  Bio
Date Reply #21 on Sat 18 Sep 2010 06:21 AM (UTC)
Message
Nick Gammon said:
Twisol said:

If we want to avoid seeing those warnings, we can compile them as separate projects under the same solution or something.

If we are unworried enough to do "something" like compiling them separately and then forgetting the warnings happened, we may as well pragma them away.

Well, it's sloppy, because it lets you put the disabling warnings just about anywhere, as you did with the setjmp one that's specific to PNG. Compiling third-party libraries separately is also good because it keeps the sources separate, reducing the potential for silly mistakes. If I recall correctly, you have a lone lua.h file in the repository, and it got out-of-date when you updated Lua.

It may be easier in the short-term, but the long-term maintenance prospects do add up. :(
'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol
Top
Posted by Nick Gammon   Australia  (23,158 posts)  Bio   Forum Administrator
Date Reply #22 on Sat 18 Sep 2010 06:32 AM (UTC)
Message
Let's move on and test the changes I have made already. This is starting to sound like the refactoring argument again. I have said, time and time again, I know the code isn't perfect, and you can always come back and say "oh dear, I found a problem here on line 7654 of file xxx.cpp". I know there are problems.

I don't want to feel that whenever I make x changes, you will come back and want x+1 changes, because that process never ends, obviously.
- Nick Gammon

www.gammon.com.au, www.mushclient.com
Top
Posted by Twisol   USA  (2,257 posts)  Bio
Date Reply #23 on Sat 18 Sep 2010 06:52 AM (UTC)

Amended on Sat 18 Sep 2010 06:57 AM (UTC) by Twisol

Message
Your current master compiles cleanly with only 193 warnings ([EDIT]: using /W3). If I add a compiler switch /D "_CRT_SECURE_NO_WARNINGS" it cuts them down to 25. (It complains that some of the functions being used don't take a buffer length, so they're susceptible to overflows.)

Going to dig into the source soon.
'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol
Top
Posted by Nick Gammon   Australia  (23,158 posts)  Bio   Forum Administrator
Date Reply #24 on Sun 19 Sep 2010 01:04 AM (UTC)
Message
I've now revamped the plugins list to change it from an MFC-style list to an STL one. This is mainly a stylistic change, but it helps move MUSHclient towards using STL containers.
- Nick Gammon

www.gammon.com.au, www.mushclient.com
Top
Posted by Nick Gammon   Australia  (23,158 posts)  Bio   Forum Administrator
Date Reply #25 on Sun 19 Sep 2010 10:48 AM (UTC)
Message
Twisol, when you send me pull requests can you please be a bit more specific about what they are addressing?

http://github.com/nickgammon/mushclient/pull/8

Your comments on the pull say what you did (well the code pretty much says the same thing) but why you did it is less clear.

You said above that you have 25 warnings. Does the commit(s) fix those warnings? Or are you just making code cleanups that you think would be nice?

Have you tested the revamped code (the new plugins interface?).

I would rather you be testing that your existing plugins still work, than go through finding the occasional variable that might not be referenced. One helps keep the client stable, the other saves a couple of bytes.
- Nick Gammon

www.gammon.com.au, www.mushclient.com
Top
Posted by Nick Gammon   Australia  (23,158 posts)  Bio   Forum Administrator
Date Reply #26 on Sun 19 Sep 2010 11:03 AM (UTC)

Amended on Sun 19 Sep 2010 11:09 AM (UTC) by Nick Gammon

Message
And can we please try to keep the discussion here? Rather than at Github? I am currently getting private emails, notes on Github, and postings here. It is hard to keep track of who thinks what, and in what order they thought it, if you start using all these different media to state your ideas.

Twisol said:

Buffer overflows are an infamous source of security holes.


Yes, I'm aware of that. And if you look at my code you'll see that I rarely if ever just blindly use strcpy without checking what I am doing.

Example 1:


pData->m_strFilename = new char [strlen (sName) + 1];
strcpy (pData->m_strFilename, sName);


I allocated memory for the string, and thus the strcpy can't overflow.

Example 2:


static char sPathName [_MAX_PATH];

  // ensure not too long
  strFileName = strFileName.Left (sizeof (sPathName) - 1);
  
  // copy to retain
  strcpy (sPathName, (const char *) strFileName);


I truncate the string to fit into the place the strcpy is going to.

Example 3:


    strcpy (filedlg.m_ofn.lpstrFile, "");


The destination field will hold an empty string.

If you want to go through and eliminate every warning, fix up every possible thing that lint might throw up, then I think you are better of writing a new client from scratch to much stricter coding standards.

I would prefer that if you are going to go through the code with a fine-tooth comb, you find things that are actually potential problems for the end-user.

For example, I found a place where I set a boolean along these lines:


m_bPluginProcessesOpenTag = m_CurrentPlugin->m_PluginCallbacks [ON_PLUGIN_MXP_OPENTAG].isvalid ();


Unfortunately, if the first plugin handles MXP open tags (which sets this flag) and the second plugin doesn't handle them, the flag is cleared, and then the first plugin didn't get the callback.

It should have read:


if (m_CurrentPlugin->m_PluginCallbacks [ON_PLUGIN_MXP_OPENTAG].isvalid ())
  m_bPluginProcessesOpenTag = true;


That way the flag is cumulative. Now that was a genuine bug. But all these warning levels didn't find that.
- Nick Gammon

www.gammon.com.au, www.mushclient.com
Top
Posted by Worstje   Netherlands  (899 posts)  Bio
Date Reply #27 on Sun 19 Sep 2010 11:53 AM (UTC)
Message
Nick Gammon said:

And can we please try to keep the discussion here? Rather than at Github? I am currently getting private emails, notes on Github, and postings here. It is hard to keep track of who thinks what, and in what order they thought it, if you start using all these different media to state your ideas.


I try to keep things here, but if I see a pull request on Github, I feel it is a bit stupid to create a new topic and drag the conversation here, since at Github you can exactly point out what lines you are talking about and in what context, while such a paper trail would be hard to follow here. Although I do admit Github tends to be a bit spammy - but the Notification Center in the Account Settings can help to lower the spam significantly.
Top
Posted by Twisol   USA  (2,257 posts)  Bio
Date Reply #28 on Sun 19 Sep 2010 06:21 PM (UTC)

Amended on Sun 19 Sep 2010 06:22 PM (UTC) by Twisol

Message
Nick Gammon said:
You said above that you have 25 warnings. Does the commit(s) fix those warnings? Or are you just making code cleanups that you think would be nice?

I spotted a few things while browsing the code, and they were easy to fix. I knew, for example, that MUSHclient had been free for quite a while now, so I expected that the registration code and order URLs were no longer used. I was correct.

Nick Gammon said:
Have you tested the revamped code (the new plugins interface?).

Informally, yes: all of the plugins I normally use still work just fine. Sorry I didn't mention it.

Nick Gammon said:
And can we please try to keep the discussion here? Rather than at Github? I am currently getting private emails, notes on Github, and postings here. It is hard to keep track of who thinks what, and in what order they thought it, if you start using all these different media to state your ideas.

I'm using the GitHub pull requests to discuss and review actual hard changes that are ready to go. It's closer to the material at hand, because you can click a tab and it'll show you the diff and affected files in a snap.

I use GitHub commit notes to point out issues with specific commits for a similar reason, except you don't even need to click to see what I'm talking about.

I discuss just about everything else here.

Like Worstje said, you can click the number next to your account name to see which notifications you've received. The news feed shown on your GitHub dashboard also collects things you're interested in, like changes to repositories you're watching.

The e-mail alerts from GitHub might be confusing, but I just use them as "Something happened on GitHub" notifications, and head to GitHub.com to dig into the details.

Nick Gammon said:
Twisol said:

Buffer overflows are an infamous source of security holes.


Yes, I'm aware of that. And if you look at my code you'll see that I rarely if ever just blindly use strcpy without checking what I am doing.

Yes, I know, on both counts. But are you really saying you never made a mistake? You yourself just fixed some bugs you both (a) introduced recently and (b) introduced a long time ago. I have every confidence in you as a programmer, Nick - especially considering that you've done much more than I ever have - but that doesn't mean I won't look at the code and ensure that things are absolutely solid.

Nick Gammon said:
If you want to go through and eliminate every warning, fix up every possible thing that lint might throw up, then I think you are better of writing a new client from scratch to much stricter coding standards.

Lint is a very different thing from standard compiler warnings, and this one is level 1. Unless warnings are disabled completely - is that even possible? - you'll always see this warning. For that matter, even using a #pragma I can't disable this warning under Release mode, which is quite bizarre. Clearly MS saw this as a very important issue, though obviously throwing an error would be taking it too far.

Nick Gammon said:
I would prefer that if you are going to go through the code with a fine-tooth comb, you find things that are actually potential problems for the end-user.

I am, frankly, confused. Are you the only one allowed to do refactorings of any sort? It would help me so much to know exactly what I am allowed to do in the source.

Nick Gammon said:
Now that was a genuine bug. But all these warning levels didn't find that.

I'm sorry Nick, but I really think you're missing the point.
'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol
Top
Posted by Worstje   Netherlands  (899 posts)  Bio
Date Reply #29 on Sun 19 Sep 2010 06:52 PM (UTC)
Message
I think I see why both sides are getting annoyed.

Twisol, you have a habit of getting carried away in technical perfection. Hell, I have that a bit myself but have in the past few years been learning to deal with it a bit (or so I hope.) Since the deviation of such an utopian perfection is repeatedly brought up in near-circular arguments, I can see why Nick is defensive.

Nick, I also see why Twisol tends to get a bit frustrated when conversing technical details with you. Given the fact MUSHclient is your baby, you are very protective of it, and with good right. But you are so protective of it that it is hard to actually contribute something worth mentioning, since anything other than trivial fixes are quickly turned into a 'refactoring might introduce bugs' discussion which leads them not being introduced.

Now, I am not going to agree with either side here. Refactoring might be good, it might be bad - it depends on the situation. The source might not be good, or you could say it is good enough. Both sides have good points to make.

In the end, let's just be mindful of eachother, and nip this war that's brooding inbetween the paragraphs in the bud, shall we?
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.

115,174 views.

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

It is now over 60 days since the last post. This thread is closed.     Refresh page

Go to topic:           Search the forum

[Go to top] top

Information and images on this site are licensed under the Creative Commons Attribution 3.0 Australia License unless stated otherwise.

Home