Message du 02/05/11 20:23 De : "Stewart, Robert" A : "'boost@lists.boost.org'" Copie à : Objet : Re: [boost] [review] string convert
Vicente BOTET wrote:
De : "Gordon Woodhull"
The interface of the proposed Convert library is not simple enough. To start out with the most annoying thing, I don't like "::from" at all. The replacement should look as close to lexical_cast as possible.
So, why the extra '::from'? Even though I disagree with the lazy evaluation (see below), even that doesn't seem to require this weird static member syntax. Is '::from' just so that '::result' will not feel lonely?
For the rest of this review, I'm going to refer to convert::from(s), but please keep in mind that I really think it should be something like string_convert(s).
I wanted to add more on the this direction. Vladimir, note that maybe the syntax proposed below could be not correct in C++, but I hope you will get the idea.
What do you think of using
* Instead of
int i1 = convert::from(str); // Throws if the conversion fails
That should be convert::from(str).
the following
int i1 = string_convert(str); // Throws if the conversion fails
You haven't specified the target type. You'd need:
You are rigth. I forget the template parameter.
int I = string_convert(str);
which doesn't read well.
Does int I = convert_to(str); reads better?
* Instead of
int i2 = convert::from(str, -1); // Returns -1 if the conversion fails
the following
int i2 = string_convert(defaults_to(str, -1)); // Returns -1 if the conversion fails
defaultts_to(T,U) is a function that generates a wrapper to T and default value U.
You have the same problems here, of course. Adding the wrapper really looks bad. I'd prefer something more like this:
int I = convert::from(str, default_(-1));
if you wish to make it explicit.
I would prefer int I = convert_to(str, default_(-1));
* Instead of
int i = convert::from(hex_str, -1) >> std::hex;
the following
int i = string_convert(defaults_to(as_istream(hex_str) >> std::hex, -1);
as_istream is a function that convert a type to an input stream (provided the type is output streamable).
I see that you're trying to make "as_istream(hex_str) >> std::hex" look like a normal stream that plugs into your string_convert function, but it looks very ugly to me.
Yes, it is ugly. Maybe the following is less ugly int i = -1; as_istream(hex_str) >> std::hex >> i ;
* Instead of
string d1 = string_convert::from(d)(locale_ = rus_locale)
std::setprecision(4) std::scientific;
the following
string d1 = string_convert(as_istream(d) >> set_locale(rus_locale) >> std::setprecision(4)
std::scientific;
here set_locale can be a manipulator.
With that locale, you could also have:
string s = convert::from(d)
set_local(rus_locale) >> std::setprecision(4) std::scientific;
or just as_istream(d) >> set_locale(rus_locale) >> std::setprecision(4) >> std::scientific >> s;
I understand that there are things hiding behind a façade:
- convert::from(d) returns a convert::result - convert::from(d) acts like a std::istream WRT manipulators - convert::from() returns a convert::converter
I also get that convert::result converts implicitly to a string but can also be queried via a safe-bool and directly for the string, but adding lots of explicit wrappers into the mix just obfuscates the desired conversion operation. It makes things more obvious, but syntactically obese.
You are right trying to force all around a single function gives non natural results. This is the main issue with this library. I'm sure we can find some more explicit, readable and acceptable syntax. Best, Vicente