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

Subject: Re: [boost] [Review] Boost.Convert library is ongoing
From: Artyom (artyomtnk_at_[hidden])
Date: 2011-04-26 11:05:53

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