SummerOfCodeProposal res

From The Battle for Wesnoth Wiki
Revision as of 15:25, 22 March 2009 by Res (talk | contribs)

A bit about me

My name is Frank Richter, currently studying Computer Sciences at the University of Jena (that's in Germany).

As far as C++ is concerned, I program in it since at least seven years now. In my spare time I also develop for Crystal Space, an open source 3D engine. While I did a lot in the topics of realtime 3D, OpenGL, shaders and such, I also did (and do) 'low-level' things like cross-platform services and, last, but in this context here now certainly not least, matters of memory optimization.

Current status

  • Upon reading the blog post linked in the idea proposal my first thought was to use a proxy class for operator[]. Below a patch that implements it.

Proxy class patch

This builds and, I believe, works, but has a number of not-so-nice issues:

  • Some places store return values in t_string&. The idea is to change these to config::proxy_string, as that type should ideally behave like t_string&.
  • config::proxy_string can cast to both std::string and t_string.

The first is necessary as t_string can cast to it as well; the second is necessary to support use of a config::proxy_string where a const t_string& is expected. Unfortunately, this introduces an ambiguity in cases where a config::proxy_string is passed to methods that can take either of std::string or t_string.

  • the variables_set class has a member (get_variable_const()) that returns a t_string&. game_state descends from this class; it's get_variable_const() implementation supposedly returns a value from a config. To ensure the ref is valid, the value as a t_string is stored in a map. This is clearly not ideal.
Index: src/config.cpp
===================================================================
--- src/config.cpp	(revision 33970)
+++ src/config.cpp	(working copy)
@@ -293,10 +293,12 @@
 	delete res;
 }
 
+/*
 t_string& config::operator[](const std::string& key)
 {
 	return values[key];
 }
+*/
 
 const t_string& config::operator[](const std::string& key) const
 {
Index: src/font.cpp
===================================================================
--- src/font.cpp	(revision 33970)
+++ src/font.cpp	(working copy)
@@ -1278,7 +1278,7 @@
 		known_fonts.insert(font["name"]);
 	}
 
-	font_order = (*fonts_config)["order"];
+	font_order = (*fonts_config)["order"].t_str();
 	const std::vector<std::string> font_order = utils::split((*fonts_config)["order"]);
 	std::vector<font::subset_descriptor> fontlist;
 	std::vector<std::string>::const_iterator font;
Index: src/playsingle_controller.cpp
===================================================================
--- src/playsingle_controller.cpp	(revision 33970)
+++ src/playsingle_controller.cpp	(working copy)
@@ -281,7 +281,7 @@
 		// Log before prestart events: they do weird things.
 		if (first_human_team_ != -1) {
 			log.start(gamestate_, teams_[first_human_team_], first_human_team_ + 1, units_,
-					  loading_game_ ? gamestate_.get_variable("turn_number") : "", status_.number_of_turns());
+					  loading_game_ ? gamestate_.get_variable("turn_number").c_str() : "", status_.number_of_turns());
 		}
 
 		fire_prestart(!loading_game_);
Index: src/gamestatus.cpp
===================================================================
--- src/gamestatus.cpp	(revision 33970)
+++ src/gamestatus.cpp	(working copy)
@@ -1166,7 +1166,7 @@
 	}
 }
 
-t_string& game_state::get_variable(const std::string& key)
+config::proxy_string game_state::get_variable(const std::string& key)
 {
 	return variable_info(key, true, variable_info::TYPE_SCALAR).as_scalar();
 }
@@ -1174,8 +1174,9 @@
 const t_string& game_state::get_variable_const(const std::string& key) const
 {
 	variable_info to_get(key, false, variable_info::TYPE_SCALAR);
-	if(!to_get.is_valid) return temporaries[key];
-	return to_get.as_scalar();
+	config::proxy_string val ((!to_get.is_valid) ? temporaries[key] : to_get.as_scalar());
+	save_t_string[key] = val.t_str();
+	return save_t_string[key];
 }
 
 config& game_state::get_variable_cfg(const std::string& key)
Index: src/theme.cpp
===================================================================
--- src/theme.cpp	(revision 33970)
+++ src/theme.cpp	(working copy)
@@ -169,13 +169,13 @@
 			// follow the inheritance hierarchy and push all the nodes on the stack
 			std::vector<const config*> parent_stack(1, (*i));
 			const config* parent;
-			const t_string* parent_id = &((**i)["inherits"]);
-			while((parent = top_cfg.find_child("resolution", "id", (*parent_id))) == NULL) {
-				parent = top_cfg.find_child("partialresolution", "id", (*parent_id));
+			t_string parent_id = ((**i)["inherits"]);
+			while((parent = top_cfg.find_child("resolution", "id", parent_id)) == NULL) {
+				parent = top_cfg.find_child("partialresolution", "id", parent_id);
 				if(parent == NULL)
-					throw config::error("[partialresolution] refers to non-existant [resolution] " + (*parent_id).str());
+					throw config::error("[partialresolution] refers to non-existant [resolution] " + parent_id.str());
 				parent_stack.push_back(parent);
-				parent_id = &((*parent)["inherits"]);
+				parent_id = ((*parent)["inherits"]);
 			}
 
 			// Add the parent resolution and apply all the modifications of its children
Index: src/variable.hpp
===================================================================
--- src/variable.hpp	(revision 33970)
+++ src/variable.hpp	(working copy)
@@ -206,7 +206,7 @@
 	 * Results: after deciding the desired type, these methods can retrieve the result
 	 * Note: first you should force_valid or check is_valid, otherwise these may fail
 	 */
-	t_string& as_scalar();
+	config::proxy_string as_scalar();
 	config& as_container();
 	array_range as_array(); //range may be empty
 };
Index: src/game_config.cpp
===================================================================
--- src/game_config.cpp	(revision 33970)
+++ src/game_config.cpp	(working copy)
@@ -239,7 +239,7 @@
 				std::string id = (**teamC)["id"];
 				std::vector<Uint32> temp = string2rgb((**teamC)["rgb"]);
 				team_rgb_range.insert(std::make_pair(id,color_range(temp)));
-				team_rgb_name[id] = (**teamC)["name"];
+				team_rgb_name[id] = (**teamC)["name"].t_str();
 				//generate palette of same name;
 				std::vector<Uint32> tp = palette(team_rgb_range[id]);
 				if(tp.size()){
Index: src/config.hpp
===================================================================
--- src/config.hpp	(revision 33970)
+++ src/config.hpp	(working copy)
@@ -150,6 +150,43 @@
 	private:
 		Itor i_;
 	};
+	
+	class proxy_string
+	{
+	  config& owner;
+	  const std::string& key;
+	  
+	  t_string& real_str() { return owner.values[key]; }
+	  const t_string& real_str() const { return owner.values[key]; }
+	public:
+	  proxy_string (config& owner, const std::string& key) : owner (owner), key (key) {}
+	
+	  proxy_string& operator= (const proxy_string& other)
+	  { return this->operator= (other.real_str()); }
+	    
+	  proxy_string& operator= (const char* str)
+	  { real_str() = str; return *this; }
+	  proxy_string& operator= (const t_string& str)
+	  { real_str() = str; return *this; }
+	  proxy_string& operator= (const std::string& str)
+	  { real_str() = str; return *this; }
+	  
+	  proxy_string& operator+= (const std::string& str)
+	  { real_str() += str; return *this; }
+	  
+	  // t_string 'emulation' methods
+	  bool empty() const { return real_str().empty(); }
+	  const char* c_str() const { return real_str().c_str(); }
+	  std::string to_serialized() const { return real_str().to_serialized(); }
+	  
+	  const std::string& str() const { return real_str().str(); }
+	  const t_string& t_str() const { return real_str(); }
+	  operator std::string () const { return real_str().str(); }
+	  operator t_string() const { return real_str(); }
+	  
+	  inline friend std::ostream& operator<<(std::ostream& os, const proxy_string& str)
+	  { return os << str.real_str(); }
+	};
 
 	typedef std::pair<const_attribute_iterator,const_attribute_iterator> const_attr_itors;
 
@@ -167,7 +204,10 @@
 	config& add_child(const std::string& key);
 	config& add_child(const std::string& key, const config& val);
 	config& add_child_at(const std::string& key, const config& val, size_t index);
-	t_string& operator[](const std::string& key);
+	
+	//t_string& operator[](const std::string& key);
+	proxy_string operator[](const std::string& key)
+	{ return proxy_string (*this, key); }
 	const t_string& operator[](const std::string& key) const;
 
 	const t_string& get_attribute(const std::string& key) const;
Index: src/game_events.cpp
===================================================================
--- src/game_events.cpp	(revision 33970)
+++ src/game_events.cpp	(working copy)
@@ -88,10 +88,10 @@
 	class pump_manager {
 		public:
 		pump_manager() :
-			x1_(state_of_game->get_variable("x1")),
-			x2_(state_of_game->get_variable("x2")),
-			y1_(state_of_game->get_variable("y1")),
-			y2_(state_of_game->get_variable("y2"))
+			x1_(state_of_game->get_variable("x1").str()),
+			x2_(state_of_game->get_variable("x2").str()),
+			y1_(state_of_game->get_variable("y1").str()),
+			y2_(state_of_game->get_variable("y2").str())
 		{
 			++instance_count;
 		}
@@ -856,7 +856,7 @@
 
 	static void store_gold_side(bool store_side, const vconfig& cfg)
 	{
-		t_string *gold_store;
+		config::proxy_string *gold_store = 0;
 		std::string side = cfg["side"];
 		std::string var_name = cfg["variable"];
 		if(var_name.empty()) {
@@ -882,11 +882,12 @@
 				state_of_game->get_variable(var_name+".user_team_name") = (*teams)[team_index].user_team_name();
 				state_of_game->get_variable(var_name+".colour") = (*teams)[team_index].map_colour_to();
 
-				gold_store = &state_of_game->get_variable(var_name+".gold");
+				gold_store = new config::proxy_string (state_of_game->get_variable(var_name+".gold"));
 			} else {
-				gold_store = &state_of_game->get_variable(var_name);
+				gold_store = new config::proxy_string (state_of_game->get_variable(var_name));
 			}
 			*gold_store = lexical_cast_default<std::string>((*teams)[team_index].gold(),"");
+			delete gold_store;
 		}
 	}
 
@@ -1158,7 +1159,7 @@
 		assert(state_of_game != NULL);
 
 		const std::string name = cfg["name"];
-		t_string& var = state_of_game->get_variable(name);
+		config::proxy_string var = state_of_game->get_variable(name);
 
 		const t_string& literal = cfg.get_attribute("literal");	// no $var substitution
 		if(literal.empty() == false) {
@@ -1191,7 +1192,7 @@
 
 		const std::string multiply = cfg["multiply"];
 		if(multiply.empty() == false) {
-			if(isint(var) && isint(multiply)) {
+			if(isint(var.str()) && isint(multiply)) {
 				var = str_cast( std::atoi(var.c_str()) * std::atoi(multiply.c_str()) );
 			} else {
 				var = str_cast( std::atof(var.c_str()) * std::atof(multiply.c_str()) );
@@ -1204,7 +1205,7 @@
 				ERR_NG << "division by zero on variable " << name << "\n";
 				return;
 			}
-			if(isint(var) && isint(divide)) {
+			if(isint(var.str()) && isint(divide)) {
 				var = str_cast( std::atoi(var.c_str()) / std::atoi(divide.c_str()) );
 			} else {
 				var = str_cast( std::atof(var.c_str()) / std::atof(divide.c_str()) );
@@ -1217,7 +1218,7 @@
 				ERR_NG << "division by zero on variable " << name << "\n";
 				return;
 			}
-			if(isint(var) && isint(modulo)) {
+			if(isint(var.str()) && isint(modulo)) {
 				var = str_cast( std::atoi(var.c_str()) % std::atoi(modulo.c_str()) );
 			} else {
 				double value = std::fmod( std::atof(var.c_str()), std::atof(modulo.c_str()) );
Index: src/gui/dialogs/addon_list.cpp
===================================================================
--- src/gui/dialogs/addon_list.cpp	(revision 33970)
+++ src/gui/dialogs/addon_list.cpp	(working copy)
@@ -74,10 +74,10 @@
 		item["label"] = tmp;
 		data.insert(std::make_pair("author", item));
 
-		item["label"] = (**itor)["downloads"];
+		item["label"] = (**itor)["downloads"].t_str();
 		data.insert(std::make_pair("downloads", item));
 
-		item["label"] = (**itor)["size"];
+		item["label"] = (**itor)["size"].t_str();
 		data.insert(std::make_pair("size", item));
 
 		list->add_row(data);
Index: src/unit_types.cpp
===================================================================
--- src/unit_types.cpp	(revision 33970)
+++ src/unit_types.cpp	(working copy)
@@ -791,8 +791,8 @@
         build_created(cfg, mv_types, races, traits);
     }
 
-	type_name_ = cfg_["name"];
-	description_ = cfg_["description"];
+	type_name_ = cfg_["name"].t_str();
+	description_ = cfg_["description"].t_str();
 	hitpoints_ = lexical_cast_default<int>(cfg["hitpoints"], 1);
 	level_ = lexical_cast_default<int>(cfg["level"], 0);
 	movement_ = lexical_cast_default<int>(cfg["movement"], 1);
Index: src/gamestatus.hpp
===================================================================
--- src/gamestatus.hpp	(revision 33970)
+++ src/gamestatus.hpp	(working copy)
@@ -120,7 +120,7 @@
 
 	// Variable access
 
-	t_string& get_variable(const std::string& varname);
+	config::proxy_string get_variable(const std::string& varname);
 	virtual const t_string& get_variable_const(const std::string& varname) const;
 	config& get_variable_cfg(const std::string& varname);
 	variable_info::array_range get_variable_cfgs(const std::string& varname);
@@ -165,6 +165,7 @@
   rand_rng::simple_rng rng_ ;
 	config variables;
 	mutable config temporaries; // lengths of arrays, etc.
+	mutable string_map save_t_string;
 	const rand_rng::set_random_generator generator_setter; /**< Make sure that rng is initialized first */
 	friend struct variable_info;
 };
Index: src/variable.cpp
===================================================================
--- src/variable.cpp	(revision 33970)
+++ src/variable.cpp	(working copy)
@@ -713,7 +713,7 @@
 	}
 }
 
-t_string& variable_info::as_scalar() {
+config::proxy_string variable_info::as_scalar() {
 	assert(is_valid);
 	return (*vars)[key];
 }
Index: src/multiplayer_connect.cpp
===================================================================
--- src/multiplayer_connect.cpp	(revision 33970)
+++ src/multiplayer_connect.cpp	(working copy)
@@ -1439,8 +1439,8 @@
 		int side_num = 1;
 		foreach (config &side, sides)
 		{
-			t_string &team_name = side["team_name"];
-			t_string &user_team_name = side["user_team_name"];
+			config::proxy_string team_name = side["team_name"];
+			config::proxy_string user_team_name = side["user_team_name"];
 
 			if(team_name.empty())
 				team_name = lexical_cast<std::string>(side_num);
@@ -1465,7 +1465,7 @@
 		foreach (config &side, sides)
 		{
 			const std::string side_num = lexical_cast<std::string>(_side_num);
-			t_string &team_name = side["team_name"];
+			config::proxy_string team_name = side["team_name"];
 
 			if(team_name.empty())
 				team_name = side_num;

Next (technically, first) Steps

  • Get rid of t_string& operator[] - could be a proxy class, as above, or like in the initial blog post, a setter function. A way in-between could be to have the proxy class for convenient migration, but mark is a deprecated; instead, promote the usage of the setter. Cases where the proxy class actually introduces ugliness should be fixed manually.
  • Determine the "variation" in strings in configs. E.g. if only very few distinct strings occur during a run of the game string deleting could be ignored and storage one-way (thus simpler). However, while I expect that there is a set of strings which occur often (keywords), there would also be lots of arbitrary strings that need proper deletion lest to introduce a leak.
Retrieved from "https://wiki.wesnoth.org/index.php?title=SummerOfCodeProposal_res&oldid=29040"