[Review] Boost.Convert library is ongoing
Since there have been no reviews of this library yet, this is another reminder that formal review of the Boost.Convert library by Vladimir Batov is ongoing and ends on May 2. If you have felt that lexical_cast is not flexible enough for use, please take a look at this library and review it. *************** * Its Purpose * *************** The library builds on the past boost::lexical_cast experience, then takes those conversion-related ideas further to suit better today's applications and programing needs. It still offers simple, minimal interface, familiar conversion behavior and additionally provides: * throwing and non-throwing conversion-failure behavior; * support for the default/fallback value to be returned when conversion fails; * two types of the conversion-failure check - basic/simple and better/safe; * formatting support based on the standard std::streams and std::stream-based manipulators (like std::hex, std::scientific, etc.); * support for different locales; * support for boost::range-compliant char and wchar_t-based containers; * no DefaultConstructibility requirement for the Target/Destination type; * consistent framework to uniformly incorporate any type-to-type conversions, extensibility and additional room to grow. With its present support for string-to-type and type-to-string conversions it is an essential tool for applications making extensive use of configuration or MS-Windows-Registry-style files or having to process/prepare considerable amounts of data in, say, XML, etc. More so, it is easily extendable to accommodate, specialize and uniformly deploy new user-defined type-to-type conversions. ******************* * Where to get it * ******************* You can get the library, using Subversion, from https://svn.boost.org/svn/boost/sandbox/convert. The HTML documentation is part of the distribution and can be found at libs/convert/index.html in the distribution above. ******************** * Writing a review * ******************** The reviews and all comments should be submitted to the developers list, and the email should have "[convert] Review" at the beginning of the subject line to make sure it's not missed. Please explicitly state in your review whether the library should be accepted. The general review checklist: - What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With what compiler? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain? And finally, every review should answer this question: - Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion. Edward Diener, Review Manager
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
Artyom <artyomtnk <at> yahoo.com> writes: Hello All, This is my review.
Artyom, Thank you for finding the time to evaluate the code and to submit your review. It's much appreciated.
I like the flexibility of this library, manipulators, locales, very-very-very convenient.
I am really glad to hear that. Especially given your evaluation runs well beyond of a quick glance over the documentation.
bool optimization.
Yes, indeed string-to-bool needs work. My only defence is that I was upfront about it as I always considered that to be more of an optimization *example* rather than a full-fledged converter. For my own purposes std::stream-based conversions are quite adequate. However, your suggestion of falling back to the std::stream-based conversion when quick string-to-bool fails makes a lot of sense. With that approach we seem to have the cake and eat it too. :-)
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 admit that these use-cases never occured to me. Still, I find them quite legitimate. Curiously (or confusingly), #1 returns '1' because convert::result is implicitly converted to bool (well, safe bool). Not good. Seems simple to fix by providing template<class T> std::ostream& operator<<(std::ostream&, typename convert<T>::result const&) I'll need to look if #3 can be addressed in a similar manner. Putting that on my TODO/FIXME list (if the review outcome is positive).
Include guards ... So it is better to write BOOST_..._HPP
Makes sense. Another entry in my TODO/FIXME list.
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 am really glad to hear that. I personally 've using the lib. quite extensively and I simply cannot imagine going back to lexical_cast... well, I just cannot as it lacks the needed functionality. I'd expect people with similar needs/tasks to face similar problems with conversions and lexical_cast.
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");
That's extra cool. I am currently working for the airline industry and they have/use UNIX (starts 1970) and industry-specific SSD (starts 1979) times. Then both UNIX and SSD are UTC and local. Or rather locals (depending on destination). Then different representations. Royal pain. That boost::locale seems to be able to help me great deal. That's the lib. that's been reviewed recently, right? I've gotta have it. Thanks for the pointer.
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
I snipped your suggestions for brevity only. They both look legitimate to me and I'll have to implement/try them both to see if there any clear advantages of one or the other. Another entry in my TODO/FIXME list. Best, V.
participants (3)
-
Artyom -
Edward Diener -
Vladimir Batov