Notice: Any messages purporting to come from this site telling you that your password has expired, or that you need to verify your details, confirm your email, resolve issues, making threats, or asking for money, are
spam. We do not email users with any such messages. If you have lost your password you can obtain a new one by using the
password reset link.
Due to spam on this forum, all posts now need moderator approval.
Entire forum
➜ MUSHclient
➜ Development
➜ Scripting engine refactoring?
Scripting engine refactoring?
|
It is now over 60 days since the last post. This thread is closed.
Refresh page
Pages: 1
2
3 4
Posted by
| Twisol
USA (2,257 posts) Bio
|
Date
| Reply #30 on Wed 08 Sep 2010 05:31 PM (UTC) Amended on Wed 08 Sep 2010 05:35 PM (UTC) by Twisol
|
Message
|
David Haley said: This isn't answering the question. What prevents you from having a full object interface without affecting the existing interface?
More OO isn't the only thing you can do under this setup - it's just the most obvious thing. (Yes, I know I'm not suggesting any other things. I just don't really feel the need to prove my plan's worth, and I have to get to Karate.)
David Haley said: You could even create some kind of Lua object that had an entry per supported plugin callback, with the difference that the parameters would be richer userdata rather than strings etc. So it wouldn't even look that different as far as script authors are concerned.
Hum. Can you explain this? A Lua object that had an entry per supported plugin callback... I'm not sure what that means.
David Haley said: I get the impression that you're going straight to the most complex solution but without even really considering the simpler solutions or what the practical tasks and goals are.
Well, do keep in mind, I came at this from a refactoring angle. This was just one of the things you could theoretically do with such a refactoring. It wasn't really like "The Lua interface reeks in MUSHclient... lets make it better by revamping the whole scripting engine support!"
I also don't think it's the most complex solution, really. The actual operations of the current WSH and Lua methods won't change except with regards to the parameters each method takes. At the most basic level, I'm (1) splitting Lua and WSH code away from each other, and (2) splitting the myriad uses of CScriptEngine::Execute() into singular methods.
As an example, to make it easier when I was on my second iteration (i.e. working out the practical difficulties by diving into the code headfirst), I defined a private method Invoke() on the WSH interface, which took a DISPPARAMS reference just as the original Execute() did. The three new Execute methods - as I hadn't come up with the one-per-callback plan - took their arguments, stored them in a DISPPARAMS, and called Invoke() with it. It was trivial to take code calling Execute() and move the DISPPARAMS manipulation into an Execute method. What I left behind was singular and unified: it didn't care what scripting engine was involved, as long as it got the arguments.
(What really killed that second iteration is that the MXP error callback takes its arguments differently based on whether you're using Lua or WSH. My current plan solves that, and it's tidy to boot.) |
'Soludra' on Achaea
Blog: http://jonathan.com/
GitHub: http://github.com/Twisol | Top |
|
Posted by
| David Haley
USA (3,881 posts) Bio
|
Date
| Reply #31 on Wed 08 Sep 2010 05:38 PM (UTC) |
Message
|
Quote: More OO isn't the only thing you can do under this setup - it's just the most obvious thing.
But that's still not answering the question. :-(
Quote: Hum. Can you explain this? A Lua object that had an entry per supported plugin callback... I'm not sure what that means.
Well, if the idea is to create some kind of rich object for interaction with MUSHclient, you could create some object that knew how to interact with the MUSHclient core, and you would add fields to the object for callbacks you want to support. The point here is just that there are alternatives to full scripting engine rewrites.
Quote: Well, do keep in mind, I came at this from a refactoring angle. This was just one of the things you could theoretically do with such a refactoring. It wasn't really like "The Lua interface reeks in MUSHclient... lets make it better by revamping the whole scripting engine support!"
Sure. But refactoring should only very rarely be done for refactoring's sake; you should always have a clear, concrete task in mind.
What has come out of this discussion is that a concrete task would be improving Lua, because we all agreed that it's better to focus on one language rather than run around trying to support a whole bunch of them with rich integration. Your original angle might have been refactoring for refactoring's sake, but as you've said yourself, discussions change things, so we should talk about where we are now, not where things were at the beginning of the thread (unless of course you want to return to that, in which case we need to talk about that too). |
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 #32 on Wed 08 Sep 2010 05:51 PM (UTC) |
Message
| I do think that my refactoring is the easiest way to accomplish this. *shrug* Now I have to run, I'll be back in a few hours. |
'Soludra' on Achaea
Blog: http://jonathan.com/
GitHub: http://github.com/Twisol | Top |
|
Posted by
| Twisol
USA (2,257 posts) Bio
|
Date
| Reply #33 on Thu 09 Sep 2010 01:07 AM (UTC) Amended on Thu 09 Sep 2010 01:24 AM (UTC) by Twisol
|
Message
| One (perhaps frivolous) advantage to being able to create new interfaces is that you can build special-purpose interfaces. I note that the trigger/alias/timer filter dialogs let you define a filter, which is run through a one-shot CScriptEngine. You could define a ListFilterScriptEngine specifically for filtering, and define useful things there.
A specific example is how you have example_filters.txt distributed with MUSHclient. With a new filter script engine, you can include these into the global environment of the filter without disturbing the normal scripting environment, so a user would just do "filter = some_predefined_filter". You can also make filters easier to understand (and write) by changing this:
function filter(name)
return GetTriggerInfo(name, 26) == "antitheft"
end
to this:
function filter(trigger)
return trigger.group == "antitheft"
end
Filters are also relatively unused (and small!), so breaking changes are probably not a huge concern here. (If they are, you can just create a Lua state same as before, and add the new stuff on top of it. No big deal.) |
'Soludra' on Achaea
Blog: http://jonathan.com/
GitHub: http://github.com/Twisol | Top |
|
Posted by
| Twisol
USA (2,257 posts) Bio
|
Date
| Reply #34 on Thu 09 Sep 2010 10:15 PM (UTC) Amended on Thu 09 Sep 2010 10:16 PM (UTC) by Twisol
|
Message
| Chronicling again:
--
I've just moved all of the Lua-specific code into LuaScriptEngine, and I've "flicked the switch" on which script engine is used:
IScriptEngine* IScriptEngine::Create (const CString& language, CMUSHclientDoc* pDoc)
{
if (language.CompareNoCase ("Lua") == 0)
return new LuaScriptEngine (pDoc);
else
return new CScriptEngine (pDoc, language);
}
MUSHclient runs fine and I'm still able to do scripting with it. None of the Execute refactoring has been done yet; so far I've simply separated the languages at the short-circuiting points in the original CScriptEngine. I have, on the other hand, moved CreateScriptEngine() into the constructor, and DisableScripting() into the destructor. A ScriptEngineException is thrown from the constructor if something goes wrong, and the destructor is hence automatically called. It also makes calling code a bit nicer, for example I'm happy with how the filtering code creates an engine:
auto_ptr<LuaScriptEngine> m_ScriptEngine = NULL; // for the filtering checks
bool bFiltering = GetFilterFlag ();
if (bFiltering)
{
try
{
m_ScriptEngine = new LuaScriptEngine (m_doc);
if (m_ScriptEngine->Parse ((LPCTSTR) GetFilterScript (), "Script file"))
bFiltering = false;
}
catch (ScriptEngineException& ex)
{
bFiltering = false;
}
catch (CException* e)
{
e->ReportError ();
e->Delete ();
bFiltering = false;
}
} // end of filtering wanted
You may notice I threw in an auto_ptr for the script engine. I just discovered those and they're really really useful, especially for RAII and destructing objects no matter what.
I wanted to keep things working while I refactored, so I left the Lua code in CScriptEngine while I did this. Now that I've "switched on" the LuaScriptEngine I can remove it, which is what I'll do next. After that I'll tackle refactoring Execute. |
'Soludra' on Achaea
Blog: http://jonathan.com/
GitHub: http://github.com/Twisol | Top |
|
Posted by
| Twisol
USA (2,257 posts) Bio
|
Date
| Reply #35 on Sat 11 Sep 2010 09:36 AM (UTC) Amended on Sat 11 Sep 2010 09:41 AM (UTC) by Twisol
|
Message
| Huh... I discovered an odd assertion caused when deleting a CScriptEngine (in CMUSHclient::DisableScripting(), the 'delete m_ScriptEngine'), which I seem to have somehow introduced in one of my first refactoring changesets. I'm honestly baffled here, I can't figure out why it would suddenly start happening. The specific error is an invalid heap pointer, and it never even gets to the engine destructor. (It asserts in _CrtIsValidHeapPointer.)
I've isolated the weirdness to one commit [1], but I can't tell what could be the problem here. The issue only happens with a WSH script engine; the Lua engine is unaffected. The only odd thing I'm doing in that commit is adding some dynamic_casts to keep things working during the refactoring, but they only happen if it's a Lua engine... and they would cause an exception there, not when the engine is being released. What the humgii is going on here? :S
[1] http://github.com/Twisol/mushclient/commit/92496a4e2bc49b0cfe8c871c55b069b4ceade068 |
'Soludra' on Achaea
Blog: http://jonathan.com/
GitHub: http://github.com/Twisol | Top |
|
Posted by
| Nick Gammon
Australia (23,158 posts) Bio
Forum Administrator |
Date
| Reply #36 on Sun 12 Sep 2010 03:08 AM (UTC) |
Message
|
Twisol said:
The only odd thing I'm doing in that commit is adding some dynamic_casts to keep things working during the refactoring ...
Casts "to keep things working" are a bit of a red flag. Even though the code which does this particular cast may not be executed in the problem case, the fact that a cast is required at all may point to the problem area. |
- Nick Gammon
www.gammon.com.au, www.mushclient.com | Top |
|
Posted by
| Twisol
USA (2,257 posts) Bio
|
Date
| Reply #37 on Sun 12 Sep 2010 03:19 AM (UTC) |
Message
| Well, I want to ensure the code continues to compile after each step. Since I haven't refactored the Execute() methods yet, it still needs to access the L data member of CScriptEngine. Once the refactoring is complete, the scaffolding won't be necessary anyways.
At any rate... if I add a LuaState() method to IScriptEngine and hide L behind it - and thus remove the necessary dynamic_casts - it still asserts. =/ |
'Soludra' on Achaea
Blog: http://jonathan.com/
GitHub: http://github.com/Twisol | Top |
|
Posted by
| Nick Gammon
Australia (23,158 posts) Bio
Forum Administrator |
Date
| Reply #38 on Sun 12 Sep 2010 03:46 AM (UTC) Amended on Sun 12 Sep 2010 04:15 AM (UTC) by Nick Gammon
|
Message
| From what I can see from your design, you essentially have this:
class CObject {
int a;
};
class IScriptEngine {
int b;
};
class CScriptEngine : public CObject, public IScriptEngine {
int c;
};
int main()
{
IScriptEngine* m_ScriptEngine = new CScriptEngine;
delete m_ScriptEngine;
return 0;
}
Now if I compile that under g++ and Valgrind it, I get this:
$ g++ test.cpp
$ valgrind ./a.out
==27318== Memcheck, a memory error detector
==27318== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
==27318== Using Valgrind-3.5.0 and LibVEX; rerun with -h for copyright info
==27318== Command: ./a.out
==27318==
==27318== Invalid free() / delete / delete[]
==27318== at 0x402354D: operator delete(void*) (vg_replace_malloc.c:346)
==27318== by 0x804853C: main (in /home/nick/development/a.out)
==27318== Address 0x42bb02c is 4 bytes inside a block of size 12 alloc'd
==27318== at 0x40243A0: operator new(unsigned int) (vg_replace_malloc.c:214)
==27318== by 0x8048510: main (in /home/nick/development/a.out)
==27318==
==27318==
==27318== HEAP SUMMARY:
==27318== in use at exit: 12 bytes in 1 blocks
==27318== total heap usage: 1 allocs, 1 frees, 12 bytes allocated
==27318==
==27318== LEAK SUMMARY:
==27318== definitely lost: 12 bytes in 1 blocks
==27318== indirectly lost: 0 bytes in 0 blocks
==27318== possibly lost: 0 bytes in 0 blocks
==27318== still reachable: 0 bytes in 0 blocks
==27318== suppressed: 0 bytes in 0 blocks
==27318== Rerun with --leak-check=full to see details of leaked memory
==27318==
==27318== For counts of detected and suppressed errors, rerun with: -v
==27318== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 17 from 6)
So, the error message is correct, and your code needs looking at. |
- Nick Gammon
www.gammon.com.au, www.mushclient.com | Top |
|
Posted by
| Twisol
USA (2,257 posts) Bio
|
Date
| Reply #39 on Sun 12 Sep 2010 04:27 AM (UTC) |
Message
| ...Feh. I forgot the most basic of things: a virtual destructor. Destructing an object through its base pointer is just about guaranteed to fail otherwise. *sigh* That should have been obvious. Thank you so much Nick. :|
virtual ~IScriptEngine() {}
|
'Soludra' on Achaea
Blog: http://jonathan.com/
GitHub: http://github.com/Twisol | Top |
|
Posted by
| David Haley
USA (3,881 posts) Bio
|
Date
| Reply #40 on Sun 12 Sep 2010 05:40 AM (UTC) |
Message
| Perhaps this is an interesting case study of how seemingly innocuous changes can create subtle bugs that took a while to track down. Be happy that in this instance the cause was easy to find because it was so easily reproducible; had it been something even more subtle this could have cost even more. This is exactly the kind of stuff that makes Nick and I uneasy about this project: unless you have a clear goal in sight, why spend all this time tracking down problems you created for yourself? |
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 #41 on Sun 12 Sep 2010 05:54 AM (UTC) Amended on Sun 12 Sep 2010 06:06 AM (UTC) by Twisol
|
Message
| Well, I'm not going to make up an excuse for what was definitely a stupid error on my part. I know that was a pretty novice mistake. All I can say is that I haven't done abstract bases in C++ before, so I hadn't worked with the technical details before; I left C++ soon after I had a passable understanding of OOP (which would be after I tried to develop a Snipes clone from scratch).
This is also why I'm keeping an audit trail of my changes with Git. I've taken your advice and collapsed related changes into one commit, which does help (even if it makes me uneasy). I was able to easily isolate the issue - I simply lacked a key point of knowledge. (I couldn't tell you what it was about Nick's post that did it, though. R-mode brain [*1] to the rescue, I guess! :P)
[*1] I'm reading "Pragmatic Thinking & Learning" from The Pragmatic Programmers. Really good book. By the way, have you heard of the broken windows hypothesis? |
'Soludra' on Achaea
Blog: http://jonathan.com/
GitHub: http://github.com/Twisol | Top |
|
Posted by
| WillFa
USA (525 posts) Bio
|
Date
| Reply #42 on Sun 12 Sep 2010 11:38 AM (UTC) |
Message
|
David Haley said:
Perhaps this is an interesting case study of how seemingly innocuous changes can create subtle bugs that took a while to track down. Be happy that in this instance the cause was easy to find because it was so easily reproducible; had it been something even more subtle this could have cost even more. This is exactly the kind of stuff that makes Nick and I uneasy about this project: unless you have a clear goal in sight, why spend all this time tracking down problems you created for yourself?
Given
1. Inheriting code is never easy.
2. MC wasn't always open source and had peer review or consistent coding standards.
3. Nick has said that he doesn't really see himself still supporting MC in 5 years.
4. and Paraphrasing him "Knowing what I know now, I'd start over" -- and the problem is that no one else knows what he knows unless they actually dive into his code.
I think it's cool that Twisol's taking the time and effort to understand Nick's code as a whole. The newer 'well-written' and the older 'spaghetti' code.
| Top |
|
Posted by
| David Haley
USA (3,881 posts) Bio
|
Date
| Reply #43 on Sun 12 Sep 2010 01:03 PM (UTC) |
Message
|
Quote: 3. Nick has said that he doesn't really see himself still supporting MC in 5 years.
Where/when did he say this? I thought that au contraire, one reason Nick is reticent to include changes is that he'd have to invest time in fully understanding them all so that he'd be able to support them later.
Quote: I think it's cool that Twisol's taking the time and effort to understand Nick's code as a whole.
No disagreement there. Taking the time to understand code is not what I've been objecting to. |
David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone
http://david.the-haleys.org | Top |
|
Posted by
| Nick Gammon
Australia (23,158 posts) Bio
Forum Administrator |
Date
| Reply #44 on Sun 12 Sep 2010 09:41 PM (UTC) |
Message
|
Nick Gammon said:
I probably won't be supporting it for the next 20 years, or even the next 10.
I said that (page 1 of this thread). Whether you read into that, that I won't be supporting it in 5 years ... well it could be read that way. I am providing free support - I am not claiming that will go on indefinitely, nor am I giving notice that I will stop supporting it on any particular date. On such a date, whenever that is, it would be handy to have other people able to at the very least answer questions, and preferably make bug fixes and add enhancements.
That is one of the reasons the source was made openly available, so that the potential exists for others to support it. Unfortunately you also need the MFC libraries which limits support to people who have purchased those.
This is one of the reasons why I suggested that, if many hours are to go into improving the code, those hours might be better spent writing a new client from scratch, that doesn't depend on proprietary libraries.
In any case, congratulations Twisol on spotting the actual problem with your assertion. Interestingly, that slowed you down a couple of days, and there is regrettably no way of knowing how many other such cases will arise, with open-ended time-frames to solve.
The ideal case would be that you promptly, efficiently and without bugs refactor the code so it is much simpler, but continues to work with existing triggers, plugins etc. |
- Nick Gammon
www.gammon.com.au, www.mushclient.com | 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.
138,955 views.
This is page 3, subject is 4 pages long:
1
2
3 4
It is now over 60 days since the last post. This thread is closed.
Refresh page
top