Hello All, This is my review.
The general review checklist:
- What is your evaluation of the design?
- I like the flexibility of this library, manipulators, locales, very-very-very convenient. - I like the relation to locale and error checking behavior - that is the Achilles' heel of the boost::lexical_cast library.
- What is your evaluation of the implementation?
In general the implementation seems fine after looking to several header files, clear. However I spotted several problems: bool optimization. ------------------ Let's say I have following numpunct: struct mynumpunct : public std::numpunct<char>{ char do_decimal_point() const { return ','; } char do_thousands_sep() const { return '.'; } std::string do_grouping() const { return "\03"; } std::string do_truename() const { return "da"; } std::string do_falsename() const { return "net"; } }; That is actually may be installed by some locale, then bool t = boost::convert<bool>::from("da"); bool f = boost::convert<bool>::from("net"); Fails which is wrong, while bool t = boost::convert<bool>::from("true"); bool f = boost::convert<bool>::from("false"); Succeeds which is fine but still it would be better to fail. I'd suggest to fallback to iostream based implementation if parsing fails or at least to fetch the values using std::use_facet<std::numpunct<char_type> >(locale).truename() or .falsename() This should be fixed. writing boost::convert<type>::result to the stream -------------------------------------------------- There is a "strange" behavior when trying to use iostreams directly after convert: // outputs 1 instead of 15000 boost::convert<int>::result r = boost::convert<int>::from("15000"); std::cout << r << std::endl; // outputs 15000 as expected std::cout << boost::convert<int>::from("15000") << std::endl; // does not compile... std::cout << boost::convert<std::string>::from(15000) << std::endl; I understand that you likely don't need to use it with iostream as you can just write to it. So there is no problem in real life but when I try to learn how to use this library it was the first thing I tried to do and it had took me some time to understand what happens. So it should be either addressed or clearly documented. Include guards -------------- Double "_" is used in many places at begin and end of include gruard "__BOOST_..._HPP__". AFAIK it is reserverd by the standard. So it is better to write BOOST_..._HPP
- What is your evaluation of the documentation?
Quite clear. However the reference documentation should be better. some parts are missing - I don't know why like convert/doc/html/header/boost/convert/string_to_bool_hpp.html
- What is your evaluation of the potential usefulness of the library?
Very useful. The conversion according to current locale of boost::lexical_cast and throwing behavior caused me to use std::stringstream as much more reliable. I implemented small tools for this in my sql library and may other places. Such library is too convenient and useful not to be in Boost.
- Did you try to use the library? With what compiler? Did you have any problems?
I've tryed it with GCC-4.3.4 on Cygwin 1.7. No problems spoted. I had also tryed it with various Boost.Locale's manipulators including parameterised and not parameterised. It worked very well allowing to do things like std::string now = boost::convert<std::string>::from(std::time(0)) >> boost::locale::as::datetime; Or std::string now = boost::convert<std::string>::from(std::time(0)) >> boost::locale::as::ftime("%Y-%m-%d %H:%M:%S"); Same for parsing - very-very convenient.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I had spend several hours mostly trying different use cases and seen how it works. Did a quick glance to various source files. And fast reading of docs. Most of the time I spend doing things with library like formatting and parsing different values. Formatting with different locales and so on.
- Are you knowledgeable about the problem domain?
Yes, I know various lexical_cast issues and had used workarounds for things this library fixes them. The most important is (locale_ = std::locale::classic()); I'm familair with iostreams, locales and manipulators as I'm the author of Boost.Locale library.
And finally, every review should answer this question:
- Do you think the library should be accepted as a Boost library?
Yes, it should be. Additional Notes: ================= Optimization and specialization part. ------------------------------------- This library optimizes "bool" case which is rarely used and also currently implemented wrong. I would suggest to do something like /// Boost.Convert framework public interface template<class TypeOut, class FastConvert=stream, class EnableOut> struct boost::convert { ... } And add two optimizations or options: stream - full version of boost_convert posix - same as stream but always use std::locale::classic() as it is very useful for Machine-to-Machine text formatting. fast - similar as above but without support of locale and manipulators. make it as fast as strtol and sprintf but locale independnet. But this is just a suggestion to possible improvement. Covenient Options For std::locale::classic() -------------------------------------------- If the suggestion above is not something to consider I'd suggest to add fast and convenient option like boost::convert<std::string>::from(13456.3455)(posix) or boost::convert<std::string>::from(13456.3455).posix() or boost::convert<std::string>::from(13456.3455).C() That is synonymous to boost::convert<std::string>::from(13456.3455)(locale_ = std::locale::classic()) Reason is that machine-to-machine text formatting is very common and actually that is what most of programmers are expecting by default. Best Regards, Artyom Beilis