Gammon Forum : MUSHclient : Development : Current plugin scoping

Log on

Gammon Forum

Entire forum

MUSHclient

Development

Current plugin scoping

Current plugin scoping

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

Pages: 1 2 3

Posted by

David Haley USA (3,881 posts)

bio

Date

Reply #15 on Tue 14 Sep 2010 07:21 PM (UTC)

Message

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.

David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone

http://david.the-haleys.org

top

Posted by

Twisol USA (2,257 posts)

bio

Date

Reply #16 on Tue 14 Sep 2010 08:52 PM (UTC)

Amended on Tue 14 Sep 2010 08:57 PM (UTC) by Twisol

Message

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.

'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol

top

Posted by

David Haley USA (3,881 posts)

bio

Date

Reply #17 on Tue 14 Sep 2010 09:10 PM (UTC)

Message

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.

David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone

http://david.the-haleys.org

top

Posted by Twisol USA (2,257 posts) bio

Date Reply #18 on Tue 14 Sep 2010 09:20 PM (UTC)

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

top

Posted by

Nick Gammon Australia (22,982 posts)

bio Forum Administrator

Date

Reply #19 on Tue 14 Sep 2010 09:57 PM (UTC)

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

top

Posted by

Nick Gammon Australia (22,982 posts)

bio Forum Administrator

Date

Reply #20 on Tue 14 Sep 2010 10:05 PM (UTC)

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

top

Posted by Twisol USA (2,257 posts) bio

Date Reply #21 on Tue 14 Sep 2010 10:07 PM (UTC)

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

top

Posted by

Twisol USA (2,257 posts)

bio

Date

Reply #22 on Tue 14 Sep 2010 10:11 PM (UTC)

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

top

Posted by

David Haley USA (3,881 posts)

bio

Date

Reply #23 on Wed 15 Sep 2010 05:51 AM (UTC)

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

top

Posted by Twisol USA (2,257 posts) bio

Date

Reply #24 on Wed 15 Sep 2010 06:14 AM (UTC)

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

top

Posted by

David Haley USA (3,881 posts)

bio

Date

Reply #25 on Wed 15 Sep 2010 05:47 PM (UTC)

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

top

Posted by

Twisol USA (2,257 posts)

bio

Date

Reply #26 on Wed 15 Sep 2010 10:00 PM (UTC)

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

top

Posted by Nick Gammon Australia (22,982 posts) bio Forum Administrator

Date Reply #27 on Wed 15 Sep 2010 10:04 PM (UTC)

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

top

Posted by

Twisol USA (2,257 posts)

bio

Date

Reply #28 on Wed 15 Sep 2010 10:23 PM (UTC)

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

top

Posted by

David Haley USA (3,881 posts)

bio

Date

Reply #29 on Wed 15 Sep 2010 10:24 PM (UTC)

Message

Twisol:

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

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.

78,709 views.

This is page 2, subject is 3 pages long: 1 2 3

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

Go to topic: Search the forum

top

Quick links: MUSHclient. MUSHclient help. Forum shortcuts. Posting templates. Lua modules. Lua documentation.

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