|
|
| (3 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>. 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. | | * 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]] |