[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]  Scripting engine refactoring?

Home  |  Users  |  Search  |  FAQ
Username:
Register forum user name
Password:
Forgotten password?
(New message)
Subject: Scripting engine refactoring?
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  4  

Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Thu 16 Sep 2010 01:04 AM (UTC)  quote  ]
Message
Okay, I've gone through this iteration a ways, and I'm hitting a few bumps that aren't issues with just CScriptEngine alone. CallPlugin, for example: I'm not honestly sure how the arguments would be passed in. I could define some kind of union type - MUSHScriptVar? - and have a list of those passed in, and that would do it, but that would also cut off the lua_xmove() technique CallScript currently uses between two Lua stacks.

Anyways, I think I'm going to take a breather right now, and look at other parts of the code. I'm not used to working so long on a single task, heh. At least I learned a lot about how the scripting internals work!

'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 Sun 12 Sep 2010 09:51 PM (UTC)  quote  ]

Amended on Sun 12 Sep 2010 10:03 PM (UTC) by Twisol

Message
Nick Gammon said:
So in this situation there is a difference between people understanding the existing code (then either they or I can support it in five years) or changing it so that only they can support it.

The latter case is more of a fork, because I know you won't pull in changes you can't support. ;) Hence, the onus is on me to produce code you can support, else my work will go to waste.

I might also add that I think the codebase is currently leaning slightly towards the "only you can support it" side of things. :)

[EDIT]: Of course, I need to know what changes you can support, and I can only know that if you're willing to give feedback on a changeset when it's ready for review.

'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 Sun 12 Sep 2010 09:48 PM (UTC)  quote  ]

Amended on Sun 12 Sep 2010 09:53 PM (UTC) by Twisol

Message
Nick Gammon said:
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.

Actually, I bought two new games from Steam, and I've been playing them when I need to mull things over. A bigger roadblock caused a correspondingly bigger delay. (And they're fun, too!)

Nick Gammon said:
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.

That is truly my goal. So long as I keep the public API unchanged, all I need to do is keep myself from introducing odd bugs. Last night I fixed an issue caused by my lack of total understanding of auto_ptr: I was initializing it explicitly with NULL, which apparently is bad. Creating one via the default constructor does the same thing, and doesn't cause a crash. :S

I think the lesson here is to be extremely careful with building blocks I've never used before.

'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 Sun 12 Sep 2010 09:47 PM (UTC)  quote  ]
Message
One last point though, I have had people on this forum before who have been highly enthusiastic, made great suggestions, and in many ways contributed to the client.

Then after three to five years they get a job, move overseas, get married, have children ... something. Maybe they just lose interest. Then we never hear from them again.

If the code is changed in major ways, so that I no longer can understand it, and also the person who makes such changes goes on to greater things (maybe they become a 3D programmer for the newest MMO game!) then no-one knows how to support the client. Such a scenario might arise in the next five years, for example.

So in this situation there is a difference between people understanding the existing code (then either they or I can support it in five years) or changing it so that only they can support it.

- 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 Sun 12 Sep 2010 09:41 PM (UTC)  quote  ]
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
[Go to top] top

Posted by David Haley   USA  (3,881 posts)  [Biography] bio   Moderator
Date Sun 12 Sep 2010 01:03 PM (UTC)  quote  ]
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
[Go to top] top

Posted by WillFa   USA  (517 posts)  [Biography] bio
Date Sun 12 Sep 2010 11:38 AM (UTC)  quote  ]
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.


[Go to top] top

Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Sun 12 Sep 2010 05:54 AM (UTC)  quote  ]

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
[Go to top] top

Posted by David Haley   USA  (3,881 posts)  [Biography] bio   Moderator
Date Sun 12 Sep 2010 05:40 AM (UTC)  quote  ]
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
[Go to top] top

Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Sun 12 Sep 2010 04:27 AM (UTC)  quote  ]
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
[Go to top] top

Posted by Nick Gammon   Australia  (18,770 posts)  [Biography] bio   Forum Administrator
Date Sun 12 Sep 2010 03:46 AM (UTC)  quote  ]

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
[Go to top] top

Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Sun 12 Sep 2010 03:19 AM (UTC)  quote  ]
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
[Go to top] top

Posted by Nick Gammon   Australia  (18,770 posts)  [Biography] bio   Forum Administrator
Date Sun 12 Sep 2010 03:08 AM (UTC)  quote  ]
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
[Go to top] top

Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Sat 11 Sep 2010 09:36 AM (UTC)  quote  ]

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
[Go to top] top

Posted by Twisol   USA  (2,229 posts)  [Biography] bio
Date Thu 09 Sep 2010 10:15 PM (UTC)  quote  ]

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


7,956 views.

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

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