From b13d0b81aedaa74ab7174cb7b7aee8536b4c8b78 Mon Sep 17 00:00:00 2001
From: twanvl <twanvl@0fc631ac-6414-0410-93d0-97cfa31319b6>
Date: Thu, 12 Oct 2006 17:08:02 +0000
Subject: [PATCH] Fixed a nasty order of destruction bug, where the memory pool
 for ScriptInts was destroyed before the PackageManager

git-svn-id: svn://svn.code.sf.net/p/magicseteditor/code/trunk@20 0fc631ac-6414-0410-93d0-97cfa31319b6
---
 src/data/field.cpp              |  4 +-
 src/main.cpp                    |  3 ++
 src/script/scriptable.cpp       | 57 +++++++++++++++++++++
 src/script/scriptable.hpp       | 89 +++++++++++++++++++++++++++++++++
 src/script/value.cpp            | 50 +++++++++++++++---
 src/script/value.hpp            | 29 +++++++----
 src/util/error.hpp              |  8 +++
 src/util/io/get_member.cpp      |  6 +--
 src/util/io/get_member.hpp      |  3 ++
 src/util/io/package.cpp         |  8 +--
 src/util/io/package_manager.cpp | 18 ++++---
 src/util/io/package_manager.hpp | 13 +++--
 src/util/smart_ptr.hpp          | 10 ++--
 13 files changed, 257 insertions(+), 41 deletions(-)
 create mode 100644 src/script/scriptable.cpp
 create mode 100644 src/script/scriptable.hpp

diff --git a/src/data/field.cpp b/src/data/field.cpp
index 50570ee5..e6b77e87 100644
--- a/src/data/field.cpp
+++ b/src/data/field.cpp
@@ -8,6 +8,7 @@
 
 #include <data/field.hpp>
 #include <data/field/text.hpp>
+#include <util/error.hpp>
 
 // ----------------------------------------------------------------------------- : Field
 
@@ -50,7 +51,8 @@ shared_ptr<Field> read_new<Field>(Reader& reader) {
 	if (type == _("text")) {
 		return new_shared<TextField>();
 	} else {
-		throw "TODO";
+		//return new_shared<TextField>();
+		throw ParseError(_("Unsupported field type: '") + type + _("'"));
 	}
 }
 
diff --git a/src/main.cpp b/src/main.cpp
index e2d40b8e..55c225ba 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -7,6 +7,7 @@
 // ----------------------------------------------------------------------------- : Includes
 
 #include <util/prec.hpp>
+#include <util/io/package_manager.hpp>
 #include <data/game.hpp>
 #include <data/set.hpp>
 #include <data/settings.hpp>
@@ -51,6 +52,7 @@ bool MSE::OnInit() {
 	} catch (...) {
 		handle_error(InternalError(_("An unexpected exception occurred, this is a bug!\nPlease save your work (use 'save as' to so you don't overwrite things),\n and restart Magic Set Editor.\n\nYou can leave a bug report on http://magicseteditor.sourceforge.net/")), false);
 	}
+	packages.destroy();
 	return false;
 }
 
@@ -58,6 +60,7 @@ bool MSE::OnInit() {
 
 int MSE::OnExit() {
 	settings.write();
+	packages.destroy();
 	return 0;
 }
 
diff --git a/src/script/scriptable.cpp b/src/script/scriptable.cpp
new file mode 100644
index 00000000..dbc3dbd6
--- /dev/null
+++ b/src/script/scriptable.cpp
@@ -0,0 +1,57 @@
+//+----------------------------------------------------------------------------+
+//| Description:  Magic Set Editor - Program to make Magic (tm) cards          |
+//| Copyright:    (C) 2001 - 2006 Twan van Laarhoven                           |
+//| License:      GNU General Public License 2 or later (see file COPYING)     |
+//+----------------------------------------------------------------------------+
+
+// ----------------------------------------------------------------------------- : Includes
+
+#include <script/scriptable.hpp>
+#include <script/context.hpp>
+#include <script/parser.hpp>
+#include <script/script.hpp>
+#include <script/value.hpp>
+
+// ----------------------------------------------------------------------------- : Store
+
+void store(const ScriptValueP& val, String& var)              { var = static_cast<String>(*val); }
+void store(const ScriptValueP& val, int&    var)              { var = *val; }
+void store(const ScriptValueP& val, double& var)              { var = *val; }
+void store(const ScriptValueP& val, bool&   var)              { var = static_cast<int>(*val); }
+void store(const ScriptValueP& val, Defaultable<String>& var) { var.assign(*val); }
+
+// ----------------------------------------------------------------------------- : OptionalScript
+
+OptionalScript::~OptionalScript() {}
+
+ScriptValueP OptionalScript::invoke(Context& ctx) {
+	if (script) {
+		return ctx.eval(*script);
+	} else {
+		return script_nil;
+	}
+}
+
+// custom reflection, different for each type
+
+template <> void Reader::handle(OptionalScript& os) {
+	handle(os.unparsed);
+	// read the script
+	os.script = parse(os.unparsed);
+}
+
+template <> void Writer::handle(const OptionalScript& os) {
+	handle(os.unparsed);
+}
+
+template <> void GetMember::handle(const OptionalScript& os) {
+	// no members
+}
+template <> void GetMember::store(const OptionalScript& os) {
+	// reflect as the script itself
+	if (os.script) {
+		store(os.script);
+	} else {
+		store(script_nil);
+	}
+}
diff --git a/src/script/scriptable.hpp b/src/script/scriptable.hpp
new file mode 100644
index 00000000..8ff92849
--- /dev/null
+++ b/src/script/scriptable.hpp
@@ -0,0 +1,89 @@
+//+----------------------------------------------------------------------------+
+//| Description:  Magic Set Editor - Program to make Magic (tm) cards          |
+//| Copyright:    (C) 2001 - 2006 Twan van Laarhoven                           |
+//| License:      GNU General Public License 2 or later (see file COPYING)     |
+//+----------------------------------------------------------------------------+
+
+#ifndef HEADER_SCRIPT_SCRIPTABLE
+#define HEADER_SCRIPT_SCRIPTABLE
+
+// ----------------------------------------------------------------------------- : Includes
+
+#include <util/prec.hpp>
+#include <util/reflect.hpp>
+#include <util/defaultable.hpp>
+
+DECLARE_INTRUSIVE_POINTER_TYPE(Script);
+class Context;
+
+// ----------------------------------------------------------------------------- : Store
+
+/// Store a ScriptValue in a variable
+void store(const ScriptValueP& val, String& var);
+void store(const ScriptValueP& val, int&    var);
+void store(const ScriptValueP& val, double& var);
+void store(const ScriptValueP& val, bool&   var);
+void store(const ScriptValueP& val, Defaultable<String>& var);
+
+// ----------------------------------------------------------------------------- : OptionalScript
+
+/// An optional script, 
+class OptionalScript {
+  public:
+	~OptionalScript();
+	/// Is the script set?
+	inline operator bool() { return !!script; }
+	
+	/// Invoke the script, return the result, or script_nil if there is no script
+	ScriptValueP invoke(Context& ctx);
+	
+	/// Invoke the script on a value
+	/** Assigns the result to value if it has changed.
+	 *  Returns true if the value has changed.
+	 */
+	template <typename T>
+	bool invokeOn(Context& ctx, T& value) {
+		if (script) {
+			T new_value;
+			store(new_value, script->invoke(ctx));
+			if (value != new_value) {
+				value = new_value;
+				return true;
+			}
+		}
+		return false;
+	}
+	
+  private:
+	ScriptP script;		///< The script, may be null if there is no script
+	String unparsed;	///< Unparsed script, for writing back to a file
+	DECLARE_REFLECTION();
+};
+
+// ----------------------------------------------------------------------------- : Scriptable
+
+/// A script that defines a calculation to find a value
+/** NOTE: reading MUST happen inside a block */
+template <typename T>
+class Scriptable {
+  public:
+	Scriptable()               : value()      {}
+	Scriptable(const T& value) : value(value) {}
+	
+	inline operator const T& () const { return value; }
+	inline bool isScripted() const { return script; }
+	
+	// Updates the value by executing the script, returns true if the value has changed
+	inline bool update(Context& ctx) {
+		return script.invokeOn(ctx, value);
+	}
+	
+  private:
+	OptionalScript script;	///< The optional script
+	T value;				///< The scripted value
+	
+	DECLARE_REFLECTION();
+};
+
+// ----------------------------------------------------------------------------- : EOF
+#endif
diff --git a/src/script/value.cpp b/src/script/value.cpp
index 5739cfe2..46514978 100644
--- a/src/script/value.cpp
+++ b/src/script/value.cpp
@@ -56,6 +56,8 @@ ScriptValueP rangeIterator(int start, int end) {
 
 // ----------------------------------------------------------------------------- : Integers
 
+#define USE_POOL_ALLOCATOR
+
 // Integer values
 class ScriptInt : public ScriptValue {
   public:
@@ -66,23 +68,59 @@ class ScriptInt : public ScriptValue {
 	virtual operator double() const { return value; }
 	virtual operator int()    const { return value; }
   protected:
+#ifdef USE_POOL_ALLOCATOR
 	virtual void destroy() {
 		boost::singleton_pool<ScriptValue, sizeof(ScriptInt)>::free(this);
 	}
+#endif
   private:
 	int value;
 };
 
+#if defined(USE_POOL_ALLOCATOR) && !defined(USE_INTRUSIVE_PTR)
+	// deallocation function for pool allocated integers
+	void destroy_value(ScriptInt* v) {
+		boost::singleton_pool<ScriptValue, sizeof(ScriptInt)>::free(v);
+	}
+#endif
+
 ScriptValueP toScript(int v) {
-//	return new_intrusive1<ScriptInt>(v);
-	return ScriptValueP(
-			new(boost::singleton_pool<ScriptValue, sizeof(ScriptInt)>::malloc())
-				ScriptInt(v));
+#ifdef USE_POOL_ALLOCATOR
+	#ifdef USE_INTRUSIVE_PTR
+		return ScriptValueP(
+				new(boost::singleton_pool<ScriptValue, sizeof(ScriptInt)>::malloc())
+					ScriptInt(v));
+	#else
+		return ScriptValueP(
+				new(boost::singleton_pool<ScriptValue, sizeof(ScriptInt)>::malloc())
+					ScriptInt(v),
+				destroy_value); // deallocation function
+	#endif
+#else
+	return new_intrusive1<ScriptInt>(v);
+#endif
 }
 
+// ----------------------------------------------------------------------------- : Booleans
+
+// Boolean values
+class ScriptBool : public ScriptValue {
+  public:
+	ScriptBool(bool v) : value(v) {}
+	virtual ScriptType type() const { return SCRIPT_INT; }
+	virtual String typeName() const { return _("boolean"); }
+	virtual operator String() const { return value ? _("true") : _("false"); }
+	virtual operator int()    const { return value; }
+  private:
+	bool value;
+};
+
 // use integers to represent true/false
-ScriptValueP script_true  = toScript((int)true);
-ScriptValueP script_false = toScript((int)false);
+/* NOTE: previous versions used ScriptInts as booleans, this gives problems
+ * when we use a pool allocator for them, because the pool is destroyed before these globals.
+ */
+ScriptValueP script_true (new ScriptBool(true));
+ScriptValueP script_false(new ScriptBool(false));
 
 
 // ----------------------------------------------------------------------------- : Doubles
diff --git a/src/script/value.hpp b/src/script/value.hpp
index 1d0464e4..ae29f8a0 100644
--- a/src/script/value.hpp
+++ b/src/script/value.hpp
@@ -11,7 +11,6 @@
 
 #include <util/prec.hpp>
 #include <util/error.hpp>
-#include <boost/intrusive_ptr.hpp>
 class Context;
 class Dependency;
 
@@ -46,7 +45,11 @@ enum ScriptType
 /// Actual values are derived types
 class ScriptValue {
   public:
-	inline ScriptValue() : refCount(0) {}
+	inline ScriptValue()
+		#ifdef USE_INTRUSIVE_PTR
+			 : refCount(0)
+		#endif
+	{}
 	virtual ~ScriptValue() {}
 	
 	/// Information on the type of this value
@@ -85,22 +88,26 @@ class ScriptValue {
 	virtual void destroy() {
 		delete this;
 	}
+#ifdef USE_INTRUSIVE_PTR
   private:
 	volatile LONG refCount;
 	friend void intrusive_ptr_add_ref(ScriptValue*);
 	friend void intrusive_ptr_release(ScriptValue*);
+#endif
 };
 
-inline void intrusive_ptr_add_ref(ScriptValue* p) {
-	//p->refCount += 1;
-	InterlockedIncrement(&p->refCount);
-}
-inline void intrusive_ptr_release(ScriptValue* p) {
-	if (InterlockedDecrement(&p->refCount) == 0) {
-	//if (--p->refCount == 0) {
-		p->destroy();
+#ifdef USE_INTRUSIVE_PTR
+	inline void intrusive_ptr_add_ref(ScriptValue* p) {
+		//p->refCount += 1;
+		InterlockedIncrement(&p->refCount);
 	}
-}
+	inline void intrusive_ptr_release(ScriptValue* p) {
+		if (InterlockedDecrement(&p->refCount) == 0) {
+		//if (--p->refCount == 0) {
+			p->destroy();
+		}
+	}
+#endif
 
 extern ScriptValueP script_nil;   ///< The preallocated nil value
 extern ScriptValueP script_true;  ///< The preallocated true value
diff --git a/src/util/error.hpp b/src/util/error.hpp
index 11376a6e..f0384cf3 100644
--- a/src/util/error.hpp
+++ b/src/util/error.hpp
@@ -64,6 +64,14 @@ class ParseError : public Error {
 	inline ParseError(const String& str) : Error(str) {}
 };
 
+/// Parse error in a particular file
+class FileParseError : public ParseError {
+  public:
+	inline FileParseError(const String& err, const String& file) :
+		ParseError(_("Error while parsing file '") + file + _("':\n") + err)
+	{}
+};
+
 /// Parse error in a script
 class ScriptParseError : public ParseError {
   public:
diff --git a/src/util/io/get_member.cpp b/src/util/io/get_member.cpp
index 421c13bb..533964ce 100644
--- a/src/util/io/get_member.cpp
+++ b/src/util/io/get_member.cpp
@@ -24,9 +24,9 @@ template <> void GetMember::store(const unsigned int& v) { value = toScript((int
 template <> void GetMember::store(const double&       v) { value = toScript(v); }
 template <> void GetMember::store(const bool&         v) { value = toScript(v); }
 
-template <> void GetMember::store(const ScriptValueP& v) { value = v; }
-template <> void GetMember::store(const ScriptP&      v) { value = v; }
-
 template <> void GetMember::store(const Vector2D&     v) {
 	value = toScript(String::Format(_("(%.10lf,%.10lf)"), v.x, v.y));
 }
+
+void GetMember::store(const ScriptValueP& v) { value = v; }
+void GetMember::store(const ScriptP&      v) { value = v; }
diff --git a/src/util/io/get_member.hpp b/src/util/io/get_member.hpp
index 317d4f76..f0393abe 100644
--- a/src/util/io/get_member.hpp
+++ b/src/util/io/get_member.hpp
@@ -12,6 +12,7 @@
 #include <util/prec.hpp>
 
 DECLARE_INTRUSIVE_POINTER_TYPE(ScriptValue);
+DECLARE_INTRUSIVE_POINTER_TYPE(Script);
 inline void intrusive_ptr_add_ref(ScriptValue* p);
 inline void intrusive_ptr_release(ScriptValue* p);
 
@@ -54,6 +55,8 @@ class GetMember {
 	template <typename T> void store(const shared_ptr<T>& pointer) {
 		value = toScript(pointer);
 	}
+	void store(const ScriptValueP&);
+	void store(const ScriptP&);
 	
   private:
 	const String& targetName;	///< The name we are looking for
diff --git a/src/util/io/package.cpp b/src/util/io/package.cpp
index 02f931d0..1337b851 100644
--- a/src/util/io/package.cpp
+++ b/src/util/io/package.cpp
@@ -379,11 +379,11 @@ Packaged::Packaged() {
 void Packaged::open(const String& package) {
 	Package::open(package);
 	Reader reader(openIn(typeName()), absoluteFilename() + _("/") + typeName());
-//	try {
+	try {
 		reader.handle(*this);
-//	} catch (const ParseError& err) {
-//		throw FileParseError(err.what(), filename+_("/")+typeName()); // more detailed message
-//	}
+	} catch (const ParseError& err) {
+		throw FileParseError(err.what(), absoluteFilename() + _("/") + typeName()); // more detailed message
+	}
 }
 void Packaged::save() {
 	//writeFile(thisT().fileName, thisT());
diff --git a/src/util/io/package_manager.cpp b/src/util/io/package_manager.cpp
index b3d73215..a50921bd 100644
--- a/src/util/io/package_manager.cpp
+++ b/src/util/io/package_manager.cpp
@@ -11,7 +11,7 @@
 
 // ----------------------------------------------------------------------------- : PackageManager
 
-String programDir() {
+String program_dir() {
 	return _("."); //TODO
 }
 
@@ -20,18 +20,22 @@ PackageManager packages;
 
 PackageManager::PackageManager() {
 	// determine data directory
-	dataDirectory = programDir();
+	data_directory = program_dir();
 	// check if this is the actual data directory, especially during debugging,
 	// the data may be higher up:
 	//  exe path  = mse/build/debug/mse.exe
 	//  data path = mse/data
-	while (!wxDirExists(dataDirectory + _("/data"))) {
-		String d = dataDirectory;
-		dataDirectory = wxPathOnly(dataDirectory);
-		if (d == dataDirectory) {
+	while (!wxDirExists(data_directory + _("/data"))) {
+		String d = data_directory;
+		data_directory = wxPathOnly(data_directory);
+		if (d == data_directory) {
 			// we are at the root -> 'data' not found anywhere in the path -> fatal error
 			throw Error(_("The MSE data files can not be found, there should be a directory called 'data' with these files"));
 		}
 	}
-	dataDirectory += _("/data");
+	data_directory += _("/data");
 }
+
+void PackageManager::destroy() {
+	loaded_packages.clear();
+}
\ No newline at end of file
diff --git a/src/util/io/package_manager.hpp b/src/util/io/package_manager.hpp
index 372fedfb..5c5a2a41 100644
--- a/src/util/io/package_manager.hpp
+++ b/src/util/io/package_manager.hpp
@@ -28,11 +28,11 @@ class PackageManager {
 	/// Open a package with the specified name (including extension)
 	template <typename T>
 	shared_ptr<T> open(const String& name) {
-		wxFileName fn(dataDirectory + _("/") + name);
+		wxFileName fn(data_directory + _("/") + name);
 		fn.Normalize();
 		String filename = fn.GetFullPath();
 		// Is this package already loaded?
-		PackagedP& p = loadedPackages[filename];
+		PackagedP& p = loaded_packages[filename];
 		shared_ptr<T> typedP = dynamic_pointer_cast<T>(p);
 		if (typedP) {
 			return typedP;
@@ -48,9 +48,14 @@ class PackageManager {
 	/// the type of package is determined by its extension!
 	PackagedP openAnyPackage(const String& filename);
 	
+	/// Empty the list of packages.
+	/** This function MUST be called before the program terminates, otherwise
+	 *  we could get into fights with pool allocators used by ScriptValues */
+	void destroy();
+	
   private:
-	map<String, PackagedP> loadedPackages;
-	String dataDirectory;
+	map<String, PackagedP> loaded_packages;
+	String data_directory;
 };
 
 /// The global PackageManager instance
diff --git a/src/util/smart_ptr.hpp b/src/util/smart_ptr.hpp
index 1224ec33..de1e1442 100644
--- a/src/util/smart_ptr.hpp
+++ b/src/util/smart_ptr.hpp
@@ -92,11 +92,11 @@ inline shared_ptr<T> new_shared7(const A0& a0, const A1& a1, const A2& a2, const
 
 #else
 	#define DECLARE_INTRUSIVE_POINTER_TYPE DECLARE_POINTER_TYPE
-	#define intrusive_ptr smart_ptr
-	#define new_intrusive  new_smart
-	#define new_intrusive1 new_smart1
-	#define new_intrusive2 new_smart2
-	#define new_intrusive3 new_smart3
+	#define intrusive_ptr shared_ptr
+	#define new_intrusive  new_shared
+	#define new_intrusive1 new_shared1
+	#define new_intrusive2 new_shared2
+	#define new_intrusive3 new_shared3
 #endif
 
 // ----------------------------------------------------------------------------- : EOF