![](https://attach.openclonk.org/avatars/112-3419.png)
Here's a fix
![](https://attach.openclonk.org/avatars/1-0778.png)
Sadly, I can really not speak of myself as an expert regarding the whole C4Aul stuff. Who can better review this patch?
Script contexts aren't deconstructed in the way you would expect - as script execution needs to push and pop them quite often, we instead have a static stack that gets reused constantly (see here).
PopContext
is indeed effectively the context destructor.
![](https://attach.openclonk.org/avatars/112-3419.png)
As long as PushContext increases it as well, yes. Otherwise the script engine would start wildly deleting all functions it executes. I suppose the argument could be made that that's the right thing to do - but it costs a little performance, and we need special code for freeing temporary scripts anyway. And so far that's the only case where the function actually gets freed after execution.
As I said below, in my opinion the bug here is that the
As I said below, in my opinion the bug here is that the
C4AulScript
destructor doesn't decrease the ref count. Style-wise, I would in hindsight replace that "bool" with a "C4AulScript *" which is NULL for non-temporary scripts. Then we could clearly reason about the script context "owning" exactly that pointer.
Huh, this looks a bit strange. The code clearly assumes that deleting the
C4AulScript
will also remove the contained function, which is what I would have expected as well (and probably expected when writing that). Looking back in the history, C4AulScript::Clear()
actually used to do that up until someone changed it. Günther, probably?
![](https://attach.openclonk.org/avatars/24-4666.png)
If anyone wants to help with review or testing, the patch is here: http://git.openclonk.org/guenther/experimental.git/commit/42608c52096ba723336540f879dfc0e6e56ee3cc
For context: In CR and earlier, functions were owned by the scripthosts representing the source they were from. In recent OC versions, functions are stored in the proplists representing definitions, the global scope, and the scenario. As usual, directexec is the odd thing left over, and doesn't receive the same amount of attention as the other stuff.
The name of this feature is "DirectExec" (the name of the internal function) or "eval" (the name of the associated user interface), by the way.
![](https://attach.openclonk.org/avatars/112-3419.png)
2. Any code changing Host or pOrgScript is unreachable by ParseFn and therefore by any DirectExec calls.
2.5 I can create a unit test for that if you fancy such things :)
3. That's why I called it something with "console":
C4Value rVal(pScript->DirectExec(pObj, szScript, "console script", false, fUseVarsFromCallerContext ? AulExec.GetContext(AulExec.GetContextDepth()-1) : NULL));
![](https://attach.openclonk.org/avatars/24-4666.png)
![](https://attach.openclonk.org/avatars/112-3419.png)
had to fix some gtest stuff. works on ubuntu and most likely on many other flavours. No clue what to do for windows.
a basic Host-equality-tester has been implemented.
![](https://attach.openclonk.org/avatars/24-4666.png)
But I'd push this even without that, thanks :-)
![](https://attach.openclonk.org/avatars/112-3419.png)
It will increase the final binary's size, if that is not an issue I can copy those lists.
btw, why are there 2 build systems next to each other?
![](https://attach.openclonk.org/avatars/24-4666.png)
There are two build systems because I'm maintaining one of them. It might eventually go away, but not until everything's ported over. Until now, CMakeLists.txt can only fully replace the various IDE-specific build systems we had for CR and early OC.
![](https://attach.openclonk.org/avatars/112-3419.png)
maybe I can port them.
![](https://attach.openclonk.org/avatars/24-4666.png)
![](https://attach.openclonk.org/avatars/112-3419.png)
but at least now there are nearly no more overlapping source files.
I hereby license the file gtest.patch under the ISC license
Attachment: gtest.patch - One big commit doing all kinds of stuff (21k)
![](https://attach.openclonk.org/avatars/24-4666.png)
![](https://attach.openclonk.org/avatars/112-3419.png)
![](https://attach.openclonk.org/avatars/24-4666.png)
![](https://attach.openclonk.org/avatars/112-3419.png)
here's a fix and another update
I hereby license the file patch0.patch under the ISC license
I hereby license the file patch1.patch under the ISC license
Attachment: patch0.patch - the fix (2k)
Attachment: patch1.patch - "make test" target (634B)
I still don't agree with building GTest from source; this is completely different from every other library we're linking against. Either we should build every single required lib from source, or we should rely on the user to provide proper link libraries and headers in the correct locations.
EXCLUDE_FROM_ALL still generates the target, it just doesn't add it as a dependency of the "all" target.
> EXCUDE_FROM_ALL should take care of that, but doesn't…
EXCLUDE_FROM_ALL still generates the target, it just doesn't add it as a dependency of the "all" target.
![](https://attach.openclonk.org/avatars/24-4666.png)
I'm sure there's some kind of reasoning behind it, but literally EVERY OTHER C++ framework manages to work just fine with precompiled libs, and I posit that their "ODR" explanation is bull unless you do some seriously strange stuff with the preprocessor. They've probably just been burned one time too many by ricing Gentoo users.
![](https://attach.openclonk.org/avatars/24-4666.png)
Still, all that was just now invented as rationalization. I actually meant that just because there wasn't a good reason for Debian to stop precompiling GTest, that decision now is still a good reason for us to compile GTest from source. As good as any "work around a problem in someone else's code" decision ever is, at least.
![](https://attach.openclonk.org/avatars/112-3419.png)
about making sure that the scripthost isn't modified by the function parsing in direct exec:
how about creating a ConstHost parallel to the Host member. use the consthost everywhere where only read access is done, and the host for write access. before calling the parsefunction you set host to nullptr and set it back to it's original value afterwards. (or something along those lines, anyway, a less messy method would be to split C4AulParse into multiple classes)
Powered by mwForum 2.29.7 © 1999-2015 Markus Wichitill