Current plugin scoping

Posted by Twisol on Mon 13 Sep 2010 05:51 AM — 33 posts, 123,327 views.

USA #0
So I was mulling over a few different things, and I remembered how I was talking to you, Nick, about making the current-plugin management cleaner. My suggestion at the time had exception safety issues. But today I think I have a solution that's much cleaner -and- has no exception safety issues. Here's the code:

class PluginScoper
{
public:
  PluginScoper (CMUSHclientDoc* doc, CPlugin* plugin)
    : _doc(doc)
  {
    _saved_plugin = _doc.m_CurrentPlugin;
    _doc.m_CurrentPlugin = plugin;
  }

  ~PluginScoper ()
  {
    _doc.m_CurrentPlugin = _saved_plugin;
  }

private:
  CMUSHclientDoc* _doc;
  CPlugin* _saved_plugin;
};


Example:
{
  PluginScoper pscope (doc, plugin);
  // stuff here

  // automatically reset here
}


If an exception occurs anywhere within the scope, the destructor is automatically called as the stack is unwound. Also, it's destructed automatically at the end of its scope anyways.


I'm not planning on using this just yet, but it seems to be a nice construct, and I'll definitely be putting it on my list of cleanup ideas. What are your thoughts?
Amended on Mon 13 Sep 2010 05:58 AM by Twisol
Australia Forum Administrator #1
Yes that looks very sensible.
USA #2
Thanks!
Australia Forum Administrator #3
My only reservation is that plugins are usually called in sequence (ie. not just a single plugin). But with some modification the general idea could be retained, but go through all plugins.

In fact, what is done a lot is to:

  • Save the current plugin
  • Loop through all plugins
  • Ignore disabled plugins
  • To non-disabled plugins, take some action
  • Restore the saved plugin


All that could usefully be encapsulated.
Australia Forum Administrator #4
Maybe your idea could just be in the inner loop, with the slight overhead of the constructor/destructor calls. But maybe there is a slightly better way.
USA #5
If you want the quick and dirty way, just do this:

{
PluginScoper pscope (m_pDoc, NULL);

// a code loop
  {
  m_pDoc->m_CurrentPlugin = foo;
  }

// automatically reset here
}


I mean, I don't like it because you still directly affect the current plugin pointer, but it would work. I could add a new method to PluginScoper to let you indirectly affect the current plugin, but that would probably have un-obvious semantics when you have two of them in the same scope.
Amended on Mon 13 Sep 2010 06:35 AM by Twisol
Australia Forum Administrator #6
You know, Twisol, if you really want to get into this you should read up on "function objects" in STL. For example:

http://gammon.com.au/forum/?id=4181

You could iterate through a list (ie. all plugins) applying a supplied function object to them (ie. the action you want to take).
Australia Forum Administrator #7
Template:post=2896
Please see the forum thread: http://gammon.com.au/forum/?id=2896.


Template:post=2902
Please see the forum thread: http://gammon.com.au/forum/?id=2902.


et. al.
USA #8
Yep, functors. I've used them before, though not extensively. I wrote a set of input/output stream manipulators you could use with cin/cout to change the text color and cursel location in the console.

I admit that I'm not sure what an elegant solution would be using functors; I haven't used them enough to have a real feel for where they fit best. I'll mull it over.
USA #9
Okay, I have another idea. Instead of using PluginScoper to both store and set the current plugin, just use it to store the current plugin. Then you manipulate the current plugin pointer how you normally would. It's kind of like an auto_ptr.

class CurrentPluginScoper
{
public:
  CurrentPluginScoper (CMUSHclientDoc* doc)
    : _doc(doc), _saved_plugin(doc.m_CurrentPlugin)
  {}

  ~CurrentPluginScoper ()
  {
    _doc.m_CurrentPlugin = _saved_plugin;
  }

private:
  CMUSHclientDoc* _doc;
  CPlugin* _saved_plugin;
};


{
CurrentPluginScoper pscope (m_pDoc);

// a code loop
  {
  m_pDoc->m_CurrentPlugin = foo;
  }

// automatically reset here
}


The whole point here is to maintain the previous current plugin in an exception-safe way, and it has the desirable byproduct that it's cleaner, since you don't have to explicitly set it back. It still gives you full ownership of what the current plugin is, letting you do your plugin loops while maintaining previous context.

I just asked myself what the simplest thing that could possibly work would be, and this is what I got.

[EDIT] The name is dreadful; I'm open to suggestions.
Amended on Mon 13 Sep 2010 07:28 AM by Twisol
USA #10
By the way, I read that Lua uses setjmp and longjmp for error handling (and coroutines), so if you use lua_error() and it skips past one of these - or really, anything that has a destructor - it'll never be destructed. That's a very, very nice pitfall that seems really easy to fall for...

[EDIT]: *facepalm* Of course, the very next e-mail I read in the mailing list archives is by Nick.
Amended on Tue 14 Sep 2010 07:35 AM by Twisol
USA #11
Quote:
CurrentPluginScoper

CurrentPluginContext? Python calls this sort of thing a 'context', although it also has more explicit syntax support.
USA #12
I was thinking about that, but it's not really a context. Nothing changes unless you set the current plugin yourself, all this thing does is guarantee that it's set back.

CurrentPluginSaver?
USA #13
I would make it so that the scoped object takes care of setting and restoring, to be explicit. Is there a reason why you'd save it, set it later, and restore it upon destruction? Why not save+set on construction and restore on destruction?
USA #14
Because of the point Nick raised before, about how he loops over many plugins rather than just one. We could have a plugin scoper in the inner loop that's constructed/destructed every time, but frankly that's a bit disgusting. You could put the plugin scoper outside the loop and start it with NULL, but then you're circumventing the class.

I thought about it, and all we really need is a way to save and restore the current plugin. We can leave the actual current-plugin management to the user code. Of course, you can do this:
PluginScoper& operator= (CPlugin* plugin)
{
  _doc->m_CurrentPlugin = plugin;
  return *this;
}


Then you'd have something like this:
{
  PluginScoper current_plugin(m_pDoc);

  // some dummy loop here
  while (true)
    current_plugin = some_plugin;
}


Honestly, I just think the semantics could get confusing if you have multiple scopers in the same scope. But if that's something we can deal with easily, well okay then.
USA #15
Quote:
We could have a plugin scoper in the inner loop that's constructed/destructed every time, but frankly that's a bit disgusting.

That's pretty strong language. What's so horrible about it, concretely? It's how Python does it and there are no problems there.

Quote:
Honestly, I just think the semantics could get confusing if you have multiple scopers in the same scope.

Sounds like you could have similarly confusing behavior no matter what approach you took if you did something strange that you're not supposed to do.
USA #16
David Haley said:
Quote:
We could have a plugin scoper in the inner loop that's constructed/destructed every time, but frankly that's a bit disgusting.

That's pretty strong language. What's so horrible about it, concretely? It's how Python does it and there are no problems there.

I don't really want to replace the current mechanic with something that performs worse. For example, putting a plugin scoper in the loop itself will set and reset the plugin back to normal every iteration. The way it is now, it only needs to reset it after everything's done, and set it once every iteration.

I'm not sure it's fair to implicate Python as a good source of design patterns for C++. The runtime environment is a different beast. If we don't care about the extra overhead, yeah, no big deal. I just don't want to make any implicit assumptions about our needs here. If Nick thinks the inner-loop scoper solution is sufficient, that's great.

David Haley said:
Quote:
Honestly, I just think the semantics could get confusing if you have multiple scopers in the same scope.

Sounds like you could have similarly confusing behavior no matter what approach you took if you did something strange that you're not supposed to do.

I'm not sure what you're saying. Are you objecting to the operator=() suggestion?

[EDIT]: Oh, I think you're just saying having multiple scopers is a bad idea. Right.
Amended on Tue 14 Sep 2010 08:57 PM by Twisol
USA #17
Quote:
I don't really want to replace the current mechanic with something that performs worse. For example, putting a plugin scoper in the loop itself will set and reset the plugin back to normal every iteration. The way it is now, it only needs to reset it after everything's done, and set it once every iteration.

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.

Quote:
Oh, I think you're just saying having multiple scopers is a bad idea. Right.

Well, basically. I'm really saying that with nearly any solution to any problem, you can still do something that you shouldn't be doing and shoot yourself in the foot.
USA #18
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.
Australia Forum Administrator #19
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.
Australia Forum Administrator #20
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.
USA #21
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.
USA #22
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.
USA #23
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)).
USA #24
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.
Amended on Wed 15 Sep 2010 06:19 AM by Twisol
USA #25
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.
USA #26
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.
Australia Forum Administrator #27
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.
USA #28
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.
USA #29
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)
USA #30
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
Amended on Wed 15 Sep 2010 10:37 PM by Twisol
Australia Forum Administrator #31
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).

USA #32
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. :)