SummerOfCodeProposal res - Revision history

Ivanovic: added category SoC

2009-04-03T22:00:11Z

added category SoC

Res: /* Current status */

2009-03-27T13:56:18Z

‎Current status

Res: /* Next (technically, first) Steps */

2009-03-23T16:46:04Z

‎Next (technically, first) Steps

Res at 14:19, 23 March 2009

2009-03-23T14:19:37Z

Show changes

Res at 17:37, 22 March 2009

2009-03-22T17:37:22Z

Res at 15:25, 22 March 2009

2009-03-22T15:25:52Z

Res: /* Current status */

2009-03-22T15:18:45Z

‎Current status

Res: New page: = 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 leas...

2009-03-22T15:13:37Z

New page: = 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 leas...

New page

= 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 <pre>t_string&</pre>. The idea is to change these to <pre>config::proxy_string</pre>, as that type should ideally behave like <pre>t_string&</pre>.
* <pre>config::proxy_string</pre> can cast to both <pre>std::string</pre> and <pre>t_string</pre>.
The first is necessary as <pre>t_string</pre> can cast to it as well; the second is necessary to
support use of a <pre>config::proxy_string</pre> where a <pre>const t_string&</pre> is expected.
Unfortunately, this introduces an ambiguity in cases where a <pre>config::proxy_string</pre> is passed to methods that can take either of <pre>std::string</pre> or <pre>t_string</pre>.
* the <pre>variables_set</pre> class has a member (<pre>get_variable_const()</pre>) that returns a <pre>t_string&</pre>. <pre>game_state</pre> descends from this class; it's <pre>get_variable_const()</pre> implementation supposedly returns a value from a <pre>config</pre>. To ensure the ref is valid, the value as a <pre>t_string</pre> is stored in a map. This is clearly not ideal.

<pre>
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;
</pre>

← Older revision		Revision as of 22:00, 3 April 2009
Line 27:		Line 27:
	* Assuming that only a limited set of keys is used all keys can be stored in a global "add-only" pool. Individual <tt>config</tt> instances would only the pointers to the global keys.		* Assuming that only a limited set of keys is used all keys can be stored in a global "add-only" pool. Individual <tt>config</tt> instances would only the pointers to the global keys.
	* For values consider changing <tt>t_string</tt> to be backed by a string that is "copy on write", so a single copy is used unless the string is manipulated. silene suggested to store short, untranslatable values inside <tt>t_string</tt> directly to use the "16 or 24 bytes" it takes up anyway.		* For values consider changing <tt>t_string</tt> to be backed by a string that is "copy on write", so a single copy is used unless the string is manipulated. silene suggested to store short, untranslatable values inside <tt>t_string</tt> directly to use the "16 or 24 bytes" it takes up anyway.
		+
		+
		+	[[Category:Summer of Code]]

@@ Line 8: / Line 8: @@
 * 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 ==
@@ Line 13: / Line 14: @@
 This builds and, I believe, works, but has a number of not-so-nice issues:
 * Some places store return values in <tt>t_string&</tt>. The idea is to change these to <tt>config::proxy_string</tt>, as that type should ideally behave like <tt>t_string&</tt>.
-* <tt>config::proxy_string</tt> can cast to both <tt>std::string</tt> and <tt>t_string</tt>.
+* <s><tt>config::proxy_string</tt> can cast to both <tt>std::string</tt> and <tt>t_string</tt>.
 The first is necessary as <tt>t_string</tt> can cast to it as well; the second is necessary to
 support use of a <tt>config::proxy_string</tt> where a <tt>const t_string&</tt> is expected.
-Unfortunately, this introduces an ambiguity in cases where a <tt>config::proxy_string</tt> is passed to methods that can take either of <tt>std::string</tt> or <tt>t_string</tt>. The ambiguity was solved in some places by using <tt>.str()</tt>. It would also go away when <tt>proxy_string operator[]</tt> is removed.
+Unfortunately, this introduces an ambiguity in cases where a <tt>config::proxy_string</tt> is passed to methods that can take either of <tt>std::string</tt> or <tt>t_string</tt>. The ambiguity was solved in some places by using <tt>.str()</tt>. It would also go away when <tt>proxy_string operator[]</tt> is removed.</s> Resolved differently, see comments on GNA.
 * the <tt>variables_set</tt> class has a member (<tt>get_variable_const()</tt>) that returns a <tt>t_string&</tt>. <tt>game_state</tt> descends from this class; it's <tt>get_variable_const()</tt> implementation supposedly returns a value from a <tt>config</tt>. To ensure the ref is valid, the value as a <tt>t_string</tt> is stored in a map. This is clearly not ideal.

← Older revision		Revision as of 16:46, 23 March 2009
Line 24:		Line 24:

	* Get rid of <tt>t_string& operator[]</tt> - 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.		* Get rid of <tt>t_string& operator[]</tt> - 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~~.	+	* Assuming that only a limited set of keys is used all keys can be stored in a global "add-only" pool. Individual <tt>config</tt> instances would only the pointers to the global keys.
		+	* For values consider changing <tt>t_string</tt> to be backed by a string that is "copy on write", so a single copy is used unless the string is manipulated. silene suggested to store short, untranslatable values inside <tt>t_string</tt> directly to use the "16 or 24 bytes" it takes up anyway.

@@ Line 16: / Line 16: @@
 The first is necessary as <tt>t_string</tt> can cast to it as well; the second is necessary to
 support use of a <tt>config::proxy_string</tt> where a <tt>const t_string&</tt> is expected.
-Unfortunately, this introduces an ambiguity in cases where a <tt>config::proxy_string</tt> is passed to methods that can take either of <tt>std::string</tt> or <tt>t_string</tt>.
+Unfortunately, this introduces an ambiguity in cases where a <tt>config::proxy_string</tt> is passed to methods that can take either of <tt>std::string</tt> or <tt>t_string</tt>. The ambiguity was solved in some places by using <tt>.str()</tt>. It would also go away when <tt>proxy_string operator[]</tt> is removed.
 * the <tt>variables_set</tt> class has a member (<tt>get_variable_const()</tt>) that returns a <tt>t_string&</tt>. <tt>game_state</tt> descends from this class; it's <tt>get_variable_const()</tt> implementation supposedly returns a value from a <tt>config</tt>. To ensure the ref is valid, the value as a <tt>t_string</tt> is stored in a map. This is clearly not ideal.

← Older revision		Revision as of 15:25, 22 March 2009
Line 367:		Line 367:
	team_name = side_num;		team_name = side_num;
	</pre>		</pre>
		+
		+	= Next (technically, first) Steps =
		+
		+	* Get rid of <tt>t_string& operator[]</tt> - 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.