Difference between revisions of "SummerOfCodeProposal res"
From The Battle for Wesnoth Wiki
(→Current status) |
|||
| 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. | ||
Revision as of 15:25, 22 March 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.
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.