[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]  Current plugin scoping
Home  |  Users  |  Search  |  FAQ
Username:
Register forum user name
Password:
Forgotten password?
(New message)
Subject: Current plugin scoping
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 

Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Wed 15 Sep 2010 10:40 PM (UTC)  [ quote  ]
Message
Well, "initially" I hadn't even considered looping at all, preferring to tackle the singular issue of ensuring that the plugin is always reset even in the face of exceptions.

I understand that it's not complex, it just didn't occur to me. :)
'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 Wed 15 Sep 2010 10:38 PM (UTC)  [ quote  ]
Message
Twisol said:

So we'd actually write that apply method (i.e. DoForEachPlugin)? I had assumed you and David were talking about std::for_each.


The for_each function isn't particularly complex. It looks like this:


template <class ITER, class F> inline
F for_each (ITER first, ITER last, F func)
  {
  for ( ; first != last; ++first)
    func (*first);
  return func;
  }


And as noted by Josuttis (http://www.josuttis.com/libbook/) the standard implementation doesn't handle the case of func deleting an item (ie. an item deleting itself).

The safe_for_each in the MUSHclient source does handle that:


template <class ITER, class F> inline
F safe_for_each (ITER first, ITER last, F func)
  {
  while (first != last)
    func (*first++);
  return func;
  }


However this itself raises an interesting point. If you are going to write DoForEachPlugin you need to account for whether not the function you pass to DoForEachPlugin might change the list (eg. delete a plugin, or change its sequence). It may not have occurred to you initially to even consider that issue (conceivably the current code doesn't either).

- Nick Gammon

www.gammon.com.au, www.mushclient.com
[Go to top] top
Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Wed 15 Sep 2010 10:35 PM (UTC)  [ quote  ]

Amended on Wed 15 Sep 2010 10:37 PM (UTC) by Twisol

Message
David Haley said:

Twisol:
Nick Gammon said:
Now if you rewrite the whole way it is done, with some sort of "apply function f to all plugins" then you expect stuff like the current plugin to be saved/restored by the auto-apply stuff, and you aren't confused any more.

This is exactly why the loop issue isn't "orthogonal". (I think you overuse that word a bit, btw. :P)


My informal definition of "orthogonal": wholly separate tasks. I see it as (1) saving the plugin and resetting it later, and (2) doing something with the current plugin. Of course, now that I understand your suggested solution, I see that it makes sense. But the loop construct will still have to solve (1) in its own way anyways. Since it's only in one or two places, it could use an explicit try/catch, even though that'll duplicate the resetting code. Or it could use my CurrentPluginScoper. The issues are orthogonal; it's how you put them together that solves the particular problem.

[EDIT] In cases that don't need looping, an explicit scoper could be used, or a dedicated function could be created that takes a functor/function pointer just like the iterating version. The power of orthogonality is that the pieces can be used separately. :D
'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol
[Go to top] top
Posted by David Haley   USA  (3,881 posts)  [Biography] bio   Moderator
Date Wed 15 Sep 2010 10:24 PM (UTC)  [ quote  ]
Message
Twisol:
Nick Gammon said:
Now if you rewrite the whole way it is done, with some sort of "apply function f to all plugins" then you expect stuff like the current plugin to be saved/restored by the auto-apply stuff, and you aren't confused any more.

This is exactly why the loop issue isn't "orthogonal". (I think you overuse that word a bit, btw. :P)
David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone

http://david.the-haleys.org
[Go to top] top
Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Wed 15 Sep 2010 10:23 PM (UTC)  [ quote  ]
Message
Nick Gammon said:
I don't think plugins throw exceptions. There are no try ... catch handlers around any of those loops. And if there is further up the calling sequence, then the current system won't reset the current plugin pointer anyway.

No, but I was really looking forward to using exceptions for exceptional cases in my script engine refactoring. It's also forward-thinking in case something ever does throw an exception when you're not looking.


Nick Gammon said:
Now if you rewrite the whole way it is done, with some sort of "apply function f to all plugins" then you expect stuff like the current plugin to be saved/restored by the auto-apply stuff, and you aren't confused any more.

So we'd actually write that apply method (i.e. DoForEachPlugin)? I had assumed you and David were talking about std::for_each. In that case, yeah, that makes sense. Then the issue is, how do we handle singular operations? Alias callbacks for example. I guess we'd just create another such function - DoForPlugin - that would have the same effect, but over one plugin.

Hmm... That's definitely cleaner in the long run. It would touch more code than my original suggestion too though, with - as you would say - no visible improvement for the user.
'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 Wed 15 Sep 2010 10:04 PM (UTC)  [ quote  ]
Message
Twisol said:

What happens if an exception is thrown before the saved plugin is reset? We need something that will automatically set it back.


I don't think plugins throw exceptions. There are no try ... catch handlers around any of those loops. And if there is further up the calling sequence, then the current system won't reset the current plugin pointer anyway.

While the idea is clever, I think it obscures what is going on, when you read the code later. For example:


{
CurrentPluginScope pscope (pDoc); // saves current plugin

// lengthy loops and processing here (eg. 60 lines)

...

// at end of scope of { ... } block then:

// destructor of pscope does an un-obvious assignment:  
// pDoc->m_CurrentPlugin = pSavedPlugin; 

}


So really, you have saved one line of code, because it currently reads like this:


{
CPlugin* pSavedPlugin = m_CurrentPlugin; // saves current plugin

// lengthy loops and processing here (eg. 60 lines)

...

// at end of scope of { ... } block then:

m_CurrentPlugin = pSavedPlugin; // restore current plugin
}


I think this just makes the work of future maintainers harder ... they think "huh? didn't the current plugin pointer get changed?". Then they (if they think to) look up what CurrentPluginScope does, and see that, aha, it saved and restored the pointer. It's obscuring behaviour for no real gain.

Now if you rewrite the whole way it is done, with some sort of "apply function f to all plugins" then you expect stuff like the current plugin to be saved/restored by the auto-apply stuff, and you aren't confused any more.
- Nick Gammon

www.gammon.com.au, www.mushclient.com
[Go to top] top
Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Wed 15 Sep 2010 10:00 PM (UTC)  [ quote  ]
Message
David Haley said:

Quote:
I don't really care about looping except for where it might cause my suggestion to be inefficient.

I'm sorry but I have trouble taking seriously problems of inefficiency when the issue in question is a single extra pointer assignment. :-/

Quote:
The point is, it's almost irrelevant to this particular issue.

No it's not -- if you're already spending all this time on refactoring you might as well do it right, not some half attempt at "skirting around the edges" as Nick said.


None of this really addresses my point, which is that the looping is orthogonal to the current-plugin maintenance. I showed that an improved looping construct still has the exact same issue I'm trying to solve here: saving and resetting the current plugin.

I proposed two solutions to making current-plugin maintenance cleaner. Both can be used within a loop; one has (indeed perhaps irrelevant) inefficiencies when used in a loop. Up to this point, we've debated what exactly the problem is. Now I'd really like to discuss actual solutions.
'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol
[Go to top] top
Posted by David Haley   USA  (3,881 posts)  [Biography] bio   Moderator
Date Wed 15 Sep 2010 05:47 PM (UTC)  [ quote  ]
Message
Quote:
I don't really care about looping except for where it might cause my suggestion to be inefficient.

I'm sorry but I have trouble taking seriously problems of inefficiency when the issue in question is a single extra pointer assignment. :-/

Quote:
The point is, it's almost irrelevant to this particular issue.

No it's not -- if you're already spending all this time on refactoring you might as well do it right, not some half attempt at "skirting around the edges" as Nick said.
David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone

http://david.the-haleys.org
[Go to top] top
Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Wed 15 Sep 2010 06:14 AM (UTC)  [ quote  ]

Amended on Wed 15 Sep 2010 06:19 AM (UTC) by Twisol

Message
But what's the implementation of do_stuff?

void do_stuff (CPlugin* plugin)
{
  CPlugin* pSavedPlugin = m_pDoc->m_CurrentPlugin;
  m_pDoc->m_CurrentPlugin = plugin;
  // where does m_pDoc come from? honest question
  // is do_stuff supposed to be a functor which takes
  // m_pDoc as a ctor parameter?

  // Do stuff

  m_pDoc->m_CurrentPlugin = pSavedPlugin;
}


As far as I can tell, it's the same issue. What happens if an exception is thrown before the saved plugin is reset? We need something that will automatically set it back. This can happen either in do_stuff, or in the code where for_each is called. The same two approaches I listed.

Looping and current-plugin saving are entirely orthogonal. I don't really care about looping except for where it might cause my suggestion to be inefficient. I'm only dealing with the issue of saving the current plugin, and resetting it when you're done with it.

That's not to say it's a bad idea to make looping better. The point is, it's almost irrelevant to this particular issue.
'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol
[Go to top] top
Posted by David Haley   USA  (3,881 posts)  [Biography] bio   Moderator
Date Wed 15 Sep 2010 05:51 AM (UTC)  [ quote  ]
Message
Quote:
The first is kinder to loops, since you set it manually. The second would need to be within the loop to be clean,

Your metrics of kindness and cleanliness are very strange to me here, but regardless, Nick is right; if you're going to go down this path in the first place (assuming it is worth going down!) there are better ways to approach the problem, such as applying a specific operation to each plugin in turn using a generic for-each loop. The handler could take care of all of this stuff, and the code at call point would be trivial: for_each(plugin, do_stuff(plugin)).
David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone

http://david.the-haleys.org
[Go to top] top
Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Tue 14 Sep 2010 10:11 PM (UTC)  [ quote  ]
Message
Nick Gammon said:
The thing is, I think we are fiddling around the edges here. If you wanted to make the plugin loops neater you would start by changing the plugin list from a CTypedPtrList to a STL list, and then use the STL for_each function to apply some operation to every plugin.

But as I've said before, the code is littered with examples of things that could be cleaned up like that. Already we are into page 2 of a thread which is discussing the pros and cons of rewriting the way a certain sort of loop is done in the code. Why stop at this particular loop design? There are probably dozens of similar ones.

In fact, I never brought up the loops. My only concern was that managing the current plugin is error-prone, because you have "wrapping" code around an operation, which can be easy to forget when you're making changes. For example, in your CPlugin::ExecutePluginScript calls, you have m_pDoc->m_CurrentPlugin = pSavedPlugin twice, once in the Lua branch and once in the WSH branch. That's very easy to mess up or forget, especially if you come back to the code later to make other changes.

All I wanted the CurrentPluginScope to do is replace the pSavedPlugin code. You should only have to save it once, and automatically have it reset when the scope ends. It makes it easier to keep track of. What happens between the saving and resetting is none of CurrentPluginScope's business.
'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 Tue 14 Sep 2010 10:07 PM (UTC)  [ quote  ]
Message
The "current plugin" mechanism itself isn't very clean, but I don't want to start making sweeping changes here. I just wanted to propose a simple technique to clean up the usage of the current plugin system.

So far there are two general approaches:

CurrentPluginScope pscope (pDoc); // saves current plugin

pDoc->m_CurrentPlugin = stuff; // this is done manually

// reset automatically


CurrentPluginScope pScope (pDoc, plugin); // saves as well as sets

// do stuff

// reset automatically


The first is kinder to loops, since you set it manually. The second would need to be within the loop to be clean, which is needless overhead. Neither really cares what you do within the "scope", it just guarantees that the current plugin will be unchanged after the scope ends.

Nick Gammon said:
*save the current plugin
*call each enabled plugin
*restore the current plugin

Exactly. I prefer the approach where CurrentPluginScope only handles steps one and three; the meat of the algorithm remains in the user code, including setting the current plugin.

Nick Gammon said:
And since you pass the document to it, conceivably as a side-effect it could do anything, so that isn't particularly clean.

We could get into making it generic RAIIScope and pass it user-defined acquire/release functors, but that's really getting out of hand. Theres no way to reset the current plugin without that CMUSHclientDoc pointer, hence it must be passed. Like I said, I don't really want to make sweeping changes here. Ideally m_CurrentPlugin wouldn't be needed at all, but that's not what I'm trying to solve.
'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 Tue 14 Sep 2010 10:05 PM (UTC)  [ quote  ]
Message
The thing is, I think we are fiddling around the edges here. If you wanted to make the plugin loops neater you would start by changing the plugin list from a CTypedPtrList to a STL list, and then use the STL for_each function to apply some operation to every plugin.

But as I've said before, the code is littered with examples of things that could be cleaned up like that. Already we are into page 2 of a thread which is discussing the pros and cons of rewriting the way a certain sort of loop is done in the code. Why stop at this particular loop design? There are probably dozens of similar ones.
- 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 Tue 14 Sep 2010 09:57 PM (UTC)  [ quote  ]
Message
I think what started as a nice idea is starting to look convoluted. Having to do something in an inner loop, which you know isn't really needed, deliberately, seems a bit silly.

Once you know there is an inner loop, forcing the construction and destruction of an object, basically to simply avoid one assignment, would make people look at that later and want to refactor it out again.

If you could easily:


  • save the current plugin
  • call each enabled plugin
  • restore the current plugin


then it would look neater.

But some plugins are simply "notification" plugins (eg. "you have connected"), some pass data down (eg. "here is a, b, c, d") and others also return values (eg. "here is the modified packet").

So a generic solution needs to allow for all of those cases, and then you start to get into the situation where the generic solution is really just hiding what is really going on, and to work out what was initially something simple, you have to now work out the behaviour supplied by CurrentPluginScope. And since you pass the document to it, conceivably as a side-effect it could do anything, so that isn't particularly clean.
- Nick Gammon

www.gammon.com.au, www.mushclient.com
[Go to top] top
Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Tue 14 Sep 2010 09:20 PM (UTC)  [ quote  ]
Message
David Haley said:
So the "disgusting" part was having an extra assignment... Surely you're not going to argue that an extra assignment per iteration with a handful of plugins is actually going to affect runtime cost in any way we care about? I'm a little surprised that you're trading symmetry of object lifetime for the "overhead" of a single assignment.

Maybe that reaction was a little visceral. It definitely looks nicer in code:
while (true)
{
  CurrentPluginScope (m_pDoc, some_plugin);
  // stuff
}


But I'm not sure what you mean by "trading symmetry of object lifetime". The basic idea is to provide a guarantee: the current plugin will be the same plugin at the end of this scope. The two implementations I've proposed just differ in who sets the current plugin.
'Soludra' on Achaea

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

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

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

5,929 views.

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

[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]