![[de]](/mwf/flags/de.png "Germany")
Direct Function calls (like InitializePlayerControls from the engine or any call from the editor console) are not cleaned up properly.Here's a fix
The C4AulScriptContext holds quite a few pointers. Would it make sense to introduce a proper deconstructor here so that the class can clean up after itself? To leave the cleanup of the fields of the class to the places where it is used (instead of the class itself) might lead to that it is forgotten at other places in the code.Sadly, I can really not speak of myself as an expert regarding the whole C4Aul stuff. Who can better review this patch?
![[gb]](/mwf/flags/gb.png "United Kingdom")
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.
so if PopContext were to decrease the refcounter of it's function object, that might work, too
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?
I should probably push the patches I've lying around that somewhat rewrite this stuff to not need temporary C4ScriptHosts, and make the cleanup happen directly in C4AulScript::DirectExec. I think I wanted to doublecheck that parsing a directexec script really doesn't modify the scripthost anymore.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.
1. The bug is obviously gone, since the function is always destroyed *yay*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));
Thanks. Adding some tests for the script engine would certainly be nice. :-)
ClickMeForPatchhad 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.
Hm, if you can stomach CMake changes, how about adding two convenience libraries with the reused source files, like Makefile.am is already doing? The source file list duplication is slowly getting out of hand...But I'd push this even without that, thanks :-)
Makefile.am is linking many src files into stuff like c4script and puncher (and others prob, too) that are not needed normally.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?
The puncher is irrelevant, since it doesn't usually get installed, and the two extra files in c4script aren't going to make a significant difference, if the linker even includes them at all. The quicker compilation time from not compiling all those source files multiple times is useful, though.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.
just for the record: what features are missing in cmake, that are in autotools?maybe I can port them.
At one point, we had some programs that were used during development in autotools, but not cmake. There are probably some configure options that aren't supported by CMake, or not in the same combinations. For some, the right answer might well be to totally remove them.
oddly enough, adding those two libraries only saves around 19 seconds of compile time (from 5min49sec) which could just as well be a random fluctuation due to other cpu usage.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)
Could you relicense the patch as ISC? (See the comment at the top of every source file)
I also added the license to the CMakeLists.txt like it already was in the other CmakeLists.txt
Thanks. I've split your patch, edited the commit messages to use the standard commit message format, and made the new test files use C4Include.h, ran the tests, and pushed the result. (All our .cpp files include that file first, and our headers are not tested without it. Given that tests are optional, not using C4Include. there would be far too fragile.)
EXCUDE_FROM_ALL should take care of that, but doesn't…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.
On Linux, every other library we use we can get from the distribution, but Debian doesn't provide pre-built GTest anymore. So there's a good reason to do this, even if one disagrees with the design choice of the GTest authors.
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.
I wouldn't call the constant boost version incompatibilities "working just fine". And hey, boost is a precedent for the GTest change: We also compile boost ourselves. When we first discussed this, I made it clear that I'd veto precompiled boost library usage because of the lack of longterm compatibility. I've also read that the Qt developers invest significant effort into keeping their C++ library working "just fine", and I can understand that the GTest developers do not want to do that. "Seriously strange" is downright common in C++: For example, C4App.h users all have to define the same USE_X11/USE_SDL_MAINLOOP/... macros in order to be compatible.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.
The DirectExec commit still has a reference to C4DirectExecScript in C4ScriptHost.h:70 (friend class …)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
