On Thu, Jul 09, 2009 at 05:31:23PM +0200, Nils Kneuper wrote:
> Hey-
>
> As Dave had discussed (http://dave.wesnoth.org/?p=9), I plan to change
> config::operator[] to config::set_attribute().
>
> The main problem with the current interface is that it requires that we hold
> actual t_strings and that references to them are stable. This greatly
> restricts
> the config class internal representation.
>
> Another solution to this problem would be using a proxy class like in
> https://gna.org/patch/?1139. I believe that the proxy class solution is too
> complicated for the benefit it provides.
I also think a proxy would be overkill.
> The main reason that I prefer set_attribute is because it is precisely what we
> want, we want to set the attribute, we don't actually need a reference to the
> attribute or something that acts like one. The proxy_string version
> significantly complicates the interface of the config class as now we add a
> class that implements basically all of t_string to config's interface.
>
> Now, there are some reasons that people may prefer the operator[] version.
> Mordante said that cfg["asd"] = "qwe" is easier to type and read than
> cfg.set_attribute("asd", "qwe"). Silene said that cfg["asd""] = "qwe" was
> easier
> to read (and thus easier to get right). I am not convinced that it is easier
> to
> read. In fact, I feel that in some cases .set_attribute() is easier to read,
> in
> others [] is easier. Consider the two lines:
> (random->add_child("random"))["value"] = lexical_cast<std::string>(res);
> random->add_child("random").set_attribute("value",
> lexical_cast<std::string>(res));
Actually I didn't object against set_attribute() and removing
t_string& operator[](const std::string& key);
I only objected against removing
const t_string& operator[](const std::string& key) const;
and thus _forcing_ to use a get_attribute() function instead.
If we add a set_attribute() it would be nice to add some overloaded
versions for bool, int, double so the caller doesn't need to do the
conversion.
> If we change to set_attribute, I see two options for getting attribute values.
> We can keep the current operator[] const for getting values, but my feeling is
> that we should then use a get_attribute(). The reasons are that,
> config::operator[] const does not act quite like std::map::operator[], and I
> think get_attribute and set_attribute are more consistent than operator[] and
> set_attribute. Also, we could have, like get and set, cast_attribute(). That
> is,
> we would replace lexical_cast<int>(cfg["key"]) with
> cfg.cast_attribute<int>("key").
As said above I prefer to keep the const version. The standard library
also doesn't have a consistent operator[] interface.
std::map::operator[] can insert new items, while std::string::operator[]
can't. (And the behaviour for std::string::operator[]() for the const
and non-const version also isn't entirely the same, when using the
string's size as index.) So a C++ developer already needs to learn the
working of operator[] for various containers.
Keeping operator[] both reduces the number of code changes and is
shorter to type. We could add a get_attribute() since users might expect
that due to set_attribute(), but I personally don't feel a great urge to
add it.
I would like to add two cast_attribute template functions:
- The first uses lexical_cast for its conversion. eg
cfg.cast_attribute<int>("key")
- The second uses lexical_cast_default for its converion. eg
cfg.cast_attribute<int>("key", 10)
I also like to consider to specialize the version for booleans which can
use utils::string_bool(). Since utils::string_bool() always has a
default value, this function can't throw bad_lexical_cast, which I don't
see as an disadvantage. If the user really needs to validate whether the
key really had a value, there's operator[] to get the contents of the
key.
--
Regards,
Mark de Wever aka Mordante/SkeletonCrew
_______________________________________________
Wesnoth-dev mailing list
[email protected]
https://mail.gna.org/listinfo/wesnoth-dev
