Difference between revisions of "SummerOfCodeProposal res"

From The Battle for Wesnoth Wiki
m (added category SoC)
 
(4 intermediate revisions by one other user not shown)
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.
 
* 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.
 +
* I submitted a proposal via the Google SoC app. It contains some other information relevant for mentors/admins.
  
 
== Proxy class patch ==
 
== Proxy class patch ==
Line 13: Line 14:
 
This builds and, I believe, works, but has a number of not-so-nice issues:
 
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>.
 
* 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
 
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.
 
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.</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.
 
* 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.
  
<pre>
+
The patch was submitted to the patch tracker: https://gna.org/patch/index.php?1139
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>
 
  
 
= Next (technically, first) Steps =
 
= 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.
 
* 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.
 +
 
 +
 
 +
[[Category:Summer of Code]]

Latest revision as of 22:00, 3 April 2009

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.
  • I submitted a proposal via the Google SoC app. It contains some other information relevant for mentors/admins.

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 ambiguity was solved in some places by using .str(). It would also go away when proxy_string operator[] is removed. Resolved differently, see comments on GNA.

  • 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.

The patch was submitted to the patch tracker: https://gna.org/patch/index.php?1139

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.
  • Assuming that only a limited set of keys is used all keys can be stored in a global "add-only" pool. Individual config instances would only the pointers to the global keys.
  • For values consider changing t_string 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 t_string directly to use the "16 or 24 bytes" it takes up anyway.
This page was last edited on 3 April 2009, at 22:00.