[boost] [review] boost::convert

Subject: [boost] [review] boost::convert
From: Jeroen Habraken (vexocide_at_[hidden])
Date: 2011-04-26 12:42:52

Hi,

My review:

-- What is your evaluation of the design?

The overall design is very flexible, there are a few points though:

  - Although this might be a matter of taste I find the choice of
operator >> to add modifiers rather odd, it smells a bit like operator
abuse. Also, it forces you to use braces in std::cout <<
(boost::convert<int>::from("12") >> std::hex) << "\n"; for example,
maybe using a member function is an option.

  - In might be nice to add a way to return a default-constructed
object to the non-throwing interface. This is useful in combination
with boost::optional: boost::convert<boost::optional<int>
>::from_default("XXX") returning an boost::optional<int>() instead of
having to write boost::convert<boost::optional<int> >::from("XXX",
boost::optional<int>());.

-- What is your evaluation of the implementation?

Had a quick read through the code, it looks fine except that names
containing "__" are reserved (used in the include guards).

-- What is your evaluation of the documentation?

The documentation covers all aspects of the library quite nicely. This
in contrast to the reference which seems mostly empty, in my opinion
it doesn't add much and can simply be removed. Since the behaviour of
convert seems to be different to lexical_cast (see below) it might be
useful to add a section listing these differences. Lastly the
documentation contains a couple of examples, it would be nice to have
these as separate files in libs/convert/example.

-- What is your evaluation of the potential usefulness of the library?

The library is very flexible and the extra features are nice, however
there is a rather large problem, in my opinion. This is the domain of
boost::lexical_cast, a library that has matured over the years and is
in heavy use. It can not be deprecated or worse, removed. Even though
it has problems (mostly missing functionality, which boost::convert
has implemented) I do not believe adding a second library is a proper
solution. Instead this functionality should be added to
boost::lexical_cast itself (and patches to do so have been proposed
over the years).

-- Did you try to use the library? With what compiler? Did you have
any problems?

Yes, on Mac OS X with both gcc 4.2.1 (the default shipped by Apple) and 4.5.2.

Problems:
  - boost::convert<int>::from("-123.2") behaves differently from
boost::lexical_cast<int>("-123.2"), the former returns -123 whilst the
latter throws. Whilst I'm not saying one is better than the other, the
differences should be documented for people switching.

  - boost::convert<int>::from('1') results in a rather long error
message (http://codepad.org/YgoR8H2t), if the conversion from X to Y
isn't possibly this should be made clear in a more user-friendly way.

-- How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

I've read the documentation, had a quick glance at the source code and
tried the basic functionality of the library.

-- Are you knowledgeable about the problem domain?

As the author of boost::coerce I'm familiar with the conversion
domain, however I'm less familiar with the locale domain.

-- Do you think the library should be accepted as a Boost library?

No it should not, instead the extra functionality should be added to
boost::lexical_cast.

Kind regards,
Jeroen Habraken