Re: [boost] [Review] Boost.Convert library is ongoing

Subject: Re: [boost] [Review] Boost.Convert library is ongoing
From: Vladimir Batov (vbatov_at_[hidden])
Date: 2011-04-27 17:48:47

> 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.