Improvements to plugin callbacks

Posted by Nick Gammon on Fri 17 Sep 2010 06:10 AM — 37 posts, 149,397 views.

Nick Gammon Australia Forum Administrator Fri 17 Sep 2010 06:10 AM #0

After all this talk of refactoring I have done a bit myself.

I have made some fairly extensive changes to the way plugin callbacks are called.

See: http://github.com/nickgammon/mushclient/commit/54797dc5f74

The main changes are:

Removed the list of DISPIDs from the CPlugin class, and replaced with a simple map of callback names to DISPID, like this:
```
map<const char *, DISPID> m_PluginCallbacks;
```
Now rather than having dozens of variables, you can simply get a DISPID (dispatch ID) by something like this:
```
foo = m_PluginCallbacks ["OnPluginChatAccept"];
```
This has allowed the simplification of quite a bit of code, and in many places rather ugly loops through each plugin are now replaced with simpler stuff like:
```
SendToAllPluginCallbacks (ON_PLUGIN_TICK);
```

Those of you who are doing private builds might like to pull this stuff in and test it. It seems to be OK, but the changes are so extensive, well, who knows?

There is scope for a bit more improvement yet, but I think that this alone makes the code look tidier and somewhat less obscure.

Twisol USA Fri 17 Sep 2010 06:38 AM #1

...

I love you.

(Platonically of course.)

Worstje Netherlands Fri 17 Sep 2010 06:39 AM #2

I'll check it out later. It definitely sounds promising. :)

Twisol USA Fri 17 Sep 2010 06:46 AM #3

Nick, could mushclient.clw be added to the .gitignore? From a cursory glance, it seems to be used simply to keep track of the last class edited with the class wizard? Like VS2005's *.vcproj.<user> files (which I have in .gitignore), it seems like something specific to one developer, not the whole project.

[EDIT]: Just finished looking at the diff, it looks a lot better! The m_CurrentPlugin saving has been clustered around only a handful of methods, which is very nice. I'm not able to compile it at the moment because I'm working in Linux currently, but I will definitely pull it down.

I do have a small nit (and it's rather unrelated to the changeset at large). 'PluginCallbacksTable' is defined in xml_load_world.cpp, but to my mind it would make more sense if it were defined in plugins.cpp.

Nick Gammon Australia Forum Administrator Fri 17 Sep 2010 10:13 PM #4

Another feature I added today was callback counts. Since the callbacks are more centralised, this is a bit easier to do.

In the Debug "summary" list you can now click on a [Cb] hyperlink to see counts, like this:


------ Callback List (alphabetic order) ------

OnPluginClose                  -> Valid: yes.  Called 0 times.
OnPluginDisable                -> Valid: yes.  Called 1 time.
OnPluginEnable                 -> Valid: yes.  Called 0 times.
OnPluginTrace                  -> Valid: yes.  Called 41 times.
4 callbacks.

This could be useful for checking if your callbacks are being called as often (or perhaps more often) as you expect.

Twisol USA Fri 17 Sep 2010 10:18 PM #5

Nifty. Since you have a separate dispid class now, and it seems to be keeping track of its own invocation count, what do you think of using it for the trigger/alias/timers too? (Or anywhere else that some nInvocationCount thing is used.)

Nick Gammon Australia Forum Administrator Fri 17 Sep 2010 11:33 PM #6

Yes, conceivably. However that is more major changes, so for the moment I'll just test plugins still work. One thing that slipped through, for example, was that hotspots stopped working.

Nick Gammon Australia Forum Administrator Sat 18 Sep 2010 03:08 AM #7

Here is my test bed plugin.


<?xml version="1.0" encoding="iso-8859-1"?>
<!DOCTYPE muclient>

<muclient>
<plugin
   name="plugin1"
   author="Nick Gammon"
   id="91b90c3974e1da6daf1e3b9e"
   language="Lua"
   purpose="Tests script callbacks"
   date_written="2010-09-18 10:49:15"
   requires="4.61"
   version="1.0"
   >

</plugin>


<!--  Script  -->


<script>
<![CDATA[

-- Tests all plugin callbacks (for version 4.62 testing)
-- Author: Nick Gammon
-- Date:   18 Sep 2010

-- what to return
retval = true

function showinfo (name, args)
  CallPlugin ("126d9061f9758498c878a204", "MsgNote", "Plugin1 got: " .. name)
  for k, v in ipairs (args) do
    CallPlugin ("126d9061f9758498c878a204", "MsgNote", " arg " .. k .. " = " .. v)
  end -- for
  CallPlugin ("126d9061f9758498c878a204", "MsgNote", "")
end -- showinfo

function OnPluginBroadcast (...)
  showinfo ("OnPluginBroadcast", {...})
  return retval
end  -- OnPluginBroadcast



function OnPluginChatAccept (...)
  showinfo ("OnPluginChatAccept", {...})
  return retval
end  -- OnPluginChatAccept



function OnPluginChatDisplay (...)
  showinfo ("OnPluginChatDisplay", {...})
  return retval
end  -- OnPluginChatDisplay



function OnPluginChatMessage (...)
  showinfo ("OnPluginChatMessage", {...})
  return retval
end  -- OnPluginChatMessage



function OnPluginChatMessageOut (...)
  showinfo ("OnPluginChatMessageOut", {...})
  return retval
end  -- OnPluginChatMessageOut



function OnPluginChatNewUser (...)
  showinfo ("OnPluginChatNewUser", {...})
  return retval
end  -- OnPluginChatNewUser



function OnPluginChatUserDisconnect (...)
  showinfo ("OnPluginChatUserDisconnect", {...})
  return retval
end  -- OnPluginChatUserDisconnect



function OnPluginClose (...)
  showinfo ("OnPluginClose", {...})
  return retval
end  -- OnPluginClose



function OnPluginCommand (...)
  showinfo ("OnPluginCommand", {...})
  return true
end  -- OnPluginCommand



function OnPluginCommandChanged (...)
  showinfo ("OnPluginCommandChanged", {...})
  return retval
end  -- OnPluginCommandChanged



function OnPluginCommandEntered (...)
  showinfo ("OnPluginCommandEntered", {...})
  return retval
end  -- OnPluginCommandEntered



function OnPluginConnect (...)
  showinfo ("OnPluginConnect", {...})
  return retval
end  -- OnPluginConnect



function OnPluginDisable (...)
  showinfo ("OnPluginDisable", {...})
  return retval
end  -- OnPluginDisable



function OnPluginDisconnect (...)
  showinfo ("OnPluginDisconnect", {...})
  return retval
end  -- OnPluginDisconnect



function OnPluginEnable (...)
  showinfo ("OnPluginEnable", {...})
  return retval
end  -- OnPluginEnable



function OnPluginGetFocus (...)
  showinfo ("OnPluginGetFocus", {...})
  return retval
end  -- OnPluginGetFocus



function OnPlugin_IAC_GA (...)
  showinfo ("OnPlugin_IAC_GA", {...})
  return retval
end  -- OnPlugin_IAC_GA



function OnPluginInstall (...)
  showinfo ("OnPluginInstall", {...})
  return retval
end  -- OnPluginInstall



function OnPluginLineReceived (...)
  showinfo ("OnPluginLineReceived", {...})
  return retval
end  -- OnPluginLineReceived



function OnPluginListChanged (...)
  showinfo ("OnPluginListChanged", {...})
  return retval
end  -- OnPluginListChanged



function OnPluginLoseFocus (...)
  showinfo ("OnPluginLoseFocus", {...})
  return retval
end  -- OnPluginLoseFocus



function OnPluginMouseMoved (...)
  showinfo ("OnPluginMouseMoved", {...})
  return retval
end  -- OnPluginMouseMoved



function OnPluginMXPcloseTag (...)
  showinfo ("OnPluginMXPcloseTag", {...})
  return retval
end  -- OnPluginMXPcloseTag



function OnPluginMXPerror (...)
  showinfo ("OnPluginMXPerror", {...})
  return retval
end  -- OnPluginMXPerror



function OnPluginMXPopenTag (...)
  showinfo ("OnPluginMXPopenTag", {...})
  return retval
end  -- OnPluginMXPopenTag



function OnPluginMXPsetEntity (...)
  showinfo ("OnPluginMXPsetEntity", {...})
  return retval
end  -- OnPluginMXPsetEntity



function OnPluginMXPsetVariable (...)
  showinfo ("OnPluginMXPsetVariable", {...})
  return retval
end  -- OnPluginMXPsetVariable



function OnPluginMXPstart (...)
  showinfo ("OnPluginMXPstart", {...})
  return retval
end  -- OnPluginMXPstart



function OnPluginMXPstop (...)
  showinfo ("OnPluginMXPstop", {...})
  return retval
end  -- OnPluginMXPstop



function OnPluginPacketReceived (...)
  showinfo ("OnPluginPacketReceived", {...})
  return retval
end  -- OnPluginPacketReceived



function OnPluginPartialLine (...)
  showinfo ("OnPluginPartialLine", {...})
  return retval
end  -- OnPluginPartialLine



function OnPluginPlaySound (...)
  showinfo ("OnPluginPlaySound", {...})
  return retval
end  -- OnPluginPlaySound



function OnPluginSaveState (...)
  showinfo ("OnPluginSaveState", {...})
  return retval
end  -- OnPluginSaveState



function OnPluginScreendraw (...)
  showinfo ("OnPluginScreendraw", {...})
  return retval
end  -- OnPluginScreendraw



function OnPluginSend (...)
  showinfo ("OnPluginSend", {...})
  return retval
end  -- OnPluginSend



function OnPluginSent (...)
  showinfo ("OnPluginSent", {...})
  return retval
end  -- OnPluginSent



function OnPluginTabComplete (...)
  showinfo ("OnPluginTabComplete", {...})
  return retval
end  -- OnPluginTabComplete



function OnPluginTelnetOption (...)
  showinfo ("OnPluginTelnetOption", {...})
  return retval
end  -- OnPluginTelnetOption



function OnPluginTelnetRequest (...)
  showinfo ("OnPluginTelnetRequest", {...})
  return retval
end  -- OnPluginTelnetRequest



function OnPluginTelnetSubnegotiation (...)
  showinfo ("OnPluginTelnetSubnegotiation", {...})
  return retval
end  -- OnPluginTelnetSubnegotiation



function OnPluginTick (...)
  showinfo ("OnPluginTick", {...})
  return retval
end  -- OnPluginTick



function OnPluginTrace (...)
  showinfo ("OnPluginTrace", {...})
  return retval
end  -- OnPluginTrace



function OnPluginWorldOutputResized (...)
  showinfo ("OnPluginWorldOutputResized", {...})
  return retval
end  -- OnPluginWorldOutputResized



function OnPluginWorldSave (...)
  showinfo ("OnPluginWorldSave", {...})
  return retval
end  -- OnPluginWorldSave
]]>
</script>


</muclient>

You'll see if you install it that you soon want to comment out some lines.

It requires the Messages_Window as described on this page:

http://www.gammon.com.au/forum/bbshowpost.php?id=10515

The basic idea is that it is a plugin will all possible callbacks in it. It shows when callbacks are called and what arguments get passed to them. You can also edit to control the return value (for example, if you return false or nil from some callbacks they suppress input and output).

I made a second one, almost identical copy, to see what happens with two plugins. Apart from changing the plugin name and plugin ID, the showinfo function was changed to:


function showinfo (name, args)
  CallPlugin ("126d9061f9758498c878a204", "MsgNote", "Plugin2 got: " .. name, "yellow")
  for k, v in ipairs (args) do
    CallPlugin ("126d9061f9758498c878a204", "MsgNote", " arg " .. k .. " = " .. v, "yellow")
  end -- for
  CallPlugin ("126d9061f9758498c878a204", "MsgNote", "")
end -- showinfo

Basically it has the different plugin name, and the messages in a different colour.

Example output:

Nick Gammon Australia Forum Administrator Sat 18 Sep 2010 03:15 AM #8

And example callbacks list with that plugin installed:


------ Callback List (alphabetic order) ------

OnPluginBroadcast              -> Valid: yes           0 calls.
OnPluginChatAccept             -> Valid: yes           0 calls.
OnPluginChatDisplay            -> Valid: yes           0 calls.
OnPluginChatMessage            -> Valid: yes           0 calls.
OnPluginChatMessageOut         -> Valid: yes           0 calls.
OnPluginChatNewUser            -> Valid: yes           0 calls.
OnPluginChatUserDisconnect     -> Valid: yes           0 calls.
OnPluginClose                  -> Valid: yes           0 calls.
OnPluginCommand                -> Valid: yes           4 calls.
OnPluginCommandChanged         -> Valid: yes          24 calls.
OnPluginCommandEntered         -> Valid: yes           4 calls.
OnPluginConnect                -> Valid: yes           0 calls.
OnPluginDisable                -> Valid: yes           1 call.
OnPluginDisconnect             -> Valid: yes           0 calls.
OnPluginEnable                 -> Valid: yes           0 calls.
OnPluginGetFocus               -> Valid: yes           2 calls.
OnPluginInstall                -> Valid: yes           1 call.
OnPluginLineReceived           -> Valid: yes          19 calls.
OnPluginListChanged            -> Valid: yes           1 call.
OnPluginLoseFocus              -> Valid: yes           0 calls.
OnPluginMXPcloseTag            -> Valid: yes           0 calls.
OnPluginMXPerror               -> Valid: yes           0 calls.
OnPluginMXPopenTag             -> Valid: yes           0 calls.
OnPluginMXPsetEntity           -> Valid: yes           0 calls.
OnPluginMXPsetVariable         -> Valid: yes           0 calls.
OnPluginMXPstart               -> Valid: yes           0 calls.
OnPluginMXPstop                -> Valid: yes           0 calls.
OnPluginMouseMoved             -> Valid: yes         864 calls.
OnPluginPacketReceived         -> Valid: yes           2 calls.
OnPluginPartialLine            -> Valid: yes         280 calls.
OnPluginPlaySound              -> Valid: yes           0 calls.
OnPluginSaveState              -> Valid: yes           0 calls.
OnPluginScreendraw             -> Valid: yes         277 calls.
OnPluginSend                   -> Valid: yes           2 calls.
OnPluginSent                   -> Valid: yes           2 calls.
OnPluginTabComplete            -> Valid: yes           0 calls.
OnPluginTelnetOption           -> Valid: yes           0 calls.
OnPluginTelnetRequest          -> Valid: yes           0 calls.
OnPluginTelnetSubnegotiation   -> Valid: yes           0 calls.
OnPluginTick                   -> Valid: yes        5017 calls.
OnPluginTrace                  -> Valid: yes           0 calls.
OnPluginWorldOutputResized     -> Valid: yes          29 calls.
OnPluginWorldSave              -> Valid: yes           1 call.
OnPlugin_IAC_GA                -> Valid: yes           0 calls.
44 callbacks.

Nick Gammon Australia Forum Administrator Sat 18 Sep 2010 05:15 AM #9

Worstje: Looking through some of your patches, why do this?


//----------------------------------------
// world.GetReceivedBytes
//----------------------------------------
static int L_GetReceivedBytes (lua_State *L)
  {
  lua_pushnumber (L, (lua_Number) (doc (L)->m_nBytesIn) );
  return 1; // number of result fields
  } // end of L_GetReceivedBytes

You are casting it to the type it already is:


LUA_API void  (lua_pushnumber) (lua_State *L, lua_Number n);

So that achieves nothing, apart from maybe documentation. But then you may as well cast everything in that case.

Twisol USA Sat 18 Sep 2010 05:24 AM #10

I'm not sure I understand your logic, Nick. CMUSHclientDoc::m_nBytesIn is an __int64. lua_Number is (by default) a double. The cast is required, because you're casting an integer to a floating-point number.

Nick Gammon Australia Forum Administrator Sat 18 Sep 2010 05:32 AM #11

Oh I see. This is another "get rid of warnings" thing. My point was, the destination is already a double.

eg.


double f;

f = 42;

You don't need to do:


f = (double) 42;

I just don't agree with littering the code with casts. In the case in point you are moving a __int64 to a double. There is going to be loss of precision potentially. So if you have warnings cranked up you get a warning. Now you can put in a cast to "artificially" get rid of the warning, or you can just suppress the warning. Either way you are saying "I know about this, don't warn me".

Worstje Netherlands Sat 18 Sep 2010 05:34 AM #12

What Twisol said.

The variable is an __int64, lua_Number (currently) is typedef'd as a double, which means it is a lossy conversion since a double cannot represent every value in an int64.

It's no different than all the others casts I felt are needed, and you feel are useless for what is ironically the same reason: preventing bugs.

If some typecast can lose data, I prefer to explicitly cast it such so I know it was intentional if ever a bug DOES crop up. If you don't add it, and you lose precision somewhere along the path... good luck finding it. :)

Edit: The difference between the two approaches is that in the warning suppression case, you lose the notification you are losing precision when you are writing new code, making it easy to miss or consciously think about. With warnings enabled, and having added the typecast, you are sure to have consciously thought about the loss in precision. With warnings suppressed, it might never cross your mind. 'It wants a number, you give it a number, right?'

Twisol USA Sat 18 Sep 2010 05:35 AM #13

It's true that you can let the compiler do it for you. That's what it does whether you cast it or not, anyways. The issue here is that disabling that warning altogether is heavy-handed; you could miss other, more important typecast warning elsewhere in the code. And if you localize the #pragma to just that one place - IIRC, #pragma pop? - you might as well just use a cast.

EDIT: There was also that ugly crash I found back when I first started with the source, stemming from a change in return value in a newer MFC version. That was pretty ugly. Typecasts at least ensure that the type going in remains the same. It may not be a big deal here, but it really is good practice.

Worstje Netherlands Sat 18 Sep 2010 05:42 AM #14

One final thing I thought needs saying... I don't think warning suppression is per definition bad. I can well see it as having a good reason to exist in for example lua_methods.cpp, since it is all gluecode that has the warnings for one and the same reason which is that lua_Number is a one-format-fits-all kind of deal.

But the issue also surfaces at a lot of other places in the code which do not have such a global reason. The NAWS negotiation is one such place I added a cast. If that one ever ends up being buggy (with increasing screen resolutions and all), at least you look at it and immediately have your attention drawn to the cast, whereas it would otherwise be very likely you stare down the algorithm for a while trying to figure out the problem.

Twisol USA Sat 18 Sep 2010 05:52 AM #15

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.

Nick Gammon Australia Forum Administrator Sat 18 Sep 2010 05:56 AM #16

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.

Twisol USA Sat 18 Sep 2010 06:02 AM #17

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.

Nick Gammon Australia Forum Administrator Sat 18 Sep 2010 06:10 AM #18

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 Australia Forum Administrator Sat 18 Sep 2010 06:15 AM #19

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.

Twisol USA Sat 18 Sep 2010 06:18 AM #20

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.

Twisol USA Sat 18 Sep 2010 06:21 AM #21

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. :(

Nick Gammon Australia Forum Administrator Sat 18 Sep 2010 06:32 AM #22

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.

Twisol USA Sat 18 Sep 2010 06:52 AM #23

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.

Nick Gammon Australia Forum Administrator Sun 19 Sep 2010 01:04 AM #24

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 Australia Forum Administrator Sun 19 Sep 2010 10:48 AM #25

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 Australia Forum Administrator Sun 19 Sep 2010 11:03 AM #26

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.

Worstje Netherlands Sun 19 Sep 2010 11:53 AM #27

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.

Twisol USA Sun 19 Sep 2010 06:21 PM #28

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.

Worstje Netherlands Sun 19 Sep 2010 06:52 PM #29

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?

Twisol USA Sun 19 Sep 2010 06:55 PM #30

Worstje said:
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.

Mmm... You're probably right. It's just hard to understand the code in its current incarnation. The majority of my non-fix changes just aim to - incrementally - fix that.

Thank you for being a calm neutral observer, hehe.

David Haley USA Mon 20 Sep 2010 12:05 AM #31

One can gain understanding of code without rewriting it to be how one would have written it.

Twisol USA Mon 20 Sep 2010 12:52 AM #32

David, didn't you say you haven't actually worked with the MUSHclient source? Advice is wonderful, but yours feels really out of context here. I -understand- the code fine these days; that doesn't mean it's easy to understand. That also doesn't mean we won't make silly mistakes that could have been prevented with encapsulation and easing out some of the coupling.

[EDIT]: Rewriting it the way I would have written it would have two end results: (1) It would break compatibility. (2) It would never get done.

David Haley USA Mon 20 Sep 2010 05:50 AM #33

I'll tell you what, Twisol: you go produce a large piece of software written such that "silly mistakes" are impossible to make, producing a framework that is magically decoupled, encapsulated and orthogonal (you can add more buzzwords if you like) while actually solving some problem for end-users. My advice is not at all out of context here. I do not work on the MUSHclient source but I have worked on a good number of large projects, both ones I started and ones inherited from other peoples, enough to know that you're not focusing on the useful tasks here and are saying things that just don't make sense. :-/ You're using magic words from some theory appealing to some aesthetic of yours, but have not actually produced some project that puts them into practice.

Your own code will always be easier for you to understand, precisely because you wrote it. Do not mistake that phenomenon for a refactoring that actually solves real-world, pragmatic problems that cause bugs in a large piece of software.

I am sorry if I sound snippy but I am a little tired of the holier-than-thou attitude that you have been projecting with respect to how things "should" be done. You keep talking about how you want everything to be this-or-that, etc., criticizing others for sloppy coding standards, poor software design, and yet you seem unwilling to actually go do this stuff yourself. So you say that the project would never get done if you go did it the way you would have wanted? Well, maybe there's a lesson there.

I would very much welcome and even encourage you to prove me wrong, and I say that sincerely. But in the meantime, there's been enough talking.

Twisol USA Mon 20 Sep 2010 06:03 AM #34

David Haley said:
yet you seem unwilling to actually go do this stuff yourself.

I'll take the other stuff, but this one seems ambiguous to me. Do you mean I'm not actually trying to do this now? The whole reason we're having this discussion is because I'm trying to do it. Do you mean actually doing my own project this way? Okay: I'll take that one too.

I think you're exaggerating my claims though. I mean, impossible to make mistakes? "magically decoupled"? I'm saying we don't have enough of these things, not that we need them 100% everywhere.

Anyways, you're right. Enough talking. At this rate I wouldn't be surprised if Nick banned me just to get some peace and quiet.

David Haley USA Mon 20 Sep 2010 06:08 AM #35

Yes, I meant doing your own project, not the changes in this thread. Re: mistakes, you said that mistakes would be prevented by encapsulation and "easing out coupling".

I think there's lots of great work that can come out of all of this. I just don't think this particular avenue is the most productive.

Twisol USA Mon 20 Sep 2010 06:14 AM #36

David Haley said:
Yes, I meant doing your own project, not the changes in this thread. Re: mistakes, you said that mistakes would be prevented by encapsulation and "easing out coupling".

Yes, I believe that a number of them can be prevented. It's like the age-old argument against globals, isn't it? The more you restrict access to a datum, the more you can be certain how it actually behaves. It moves the onus of good behavior from the programmer to the compiler. But I certainly don't expect these things to make mistakes or bugs impossible... That would be silly.

I guess I'll be spending more time on Aspect, at any rate. :)