[boost] [review] Convert library

Subject: [boost] [review] Convert library
From: Jeroen Habraken (vexocide_at_[hidden])
Date: 2014-05-24 15:08:51

Hi,

As promised my review of the boost::convert library. Note that the
questions were answered in a seemingly random order, thus I hope it all
comes together. Secondly, being the author of the boost::coerce library
[2][3] I may be a little biased at times, please forgive me.

-- What is your evaluation of the design?

There are two parts to this, the design of the backend and the design of
the interface that the user predominantly sees. I have little to say on the
first, the notion of multiple backend converters is a big step forwards and
their interface is sane.

I'm far less convinced when it comes to the user-facing interface as I
believe the simple tasks should be simple and intuitive, whereas the harder
tasks may be harder.

The most trivial use of boost::convert, and boost::lexical_cast and
boost::coerce simply don't compare in my opinion.

    boost::cstringstream_converter cnv;
    int i = boost::convert<int>::from("23", cnv).value();

versus

    int i = boost::lexical_cast<int>("23");
    int j = boost::coerce::as<int>("23");

When the convert library makes it into boost I'd want to mentally deprecate
lexical_cast as a library I should use, and always use convert instead.
This means convert should have a default converter, which may well be
backed by lexical_cast with an interface such as:

    int i = boost::convert<int>::from("23").value();

This still doesn't completely satisfy me as I'd like to write

    int i = boost::convert<int>::from("23");

but it'd be an improvement in my opinion. Now I'm aware lexical_cast
doesn't have a non-throwing interface, and it probably never will, but I've
solved the problem as follows in boost::coerce:

    int i = boost::coerce::as_default<int>("XXX");
    int j = boost::coerce::as_default<int>("XXX", tag::hex());
    int k = boost::coerce::as_default<int>("XXX", 23);
    int l = boost::coerce::as_default<int>("XXX", tag::hex(), 23);

The former two returning the default-constructed int, 0, the latter two
returning 23. That said, I'll be the first to admit the coerce interface
isn't perfect, the fact that I need SFINAE to distinguish between
as_default calls with two parameters, the second either being a tag
(somewhat similar to your converters but not quite) or the default value
goes to show just that. However I'd prefer

    int i = boost::convert<int>::from("23", cnv);
    int i = boost::convert<int>::from_default("23", cnv, 0);

to the current interface using value_or(0). This is subjective though, and
quite possibly a matter of taste.

In summary, before this turns into too long a rant, I think the current
user-facing interface can be improved substantially, but I'm well aware
this is subjective.

-- What is your evaluation of the implementation?

I've only had a quick glance at the code and it still looks fine. The gripe
I had two years ago regarding the use of reserved names (include guards
prefixed with "__") has been fixed. A few comments though:

In the boost::lexical_cast based converter there is a try { } catch (...) {
} which I believe to be an overly broad catch as lexical_cast explicitly
throws a boost::bad_lexical_cast. If in an unlikely event a std::bad_alloc
were to be thrown for example I'd rather not have boost::convert hide it
from me.

In the std::strtol based converter errno isn't checked, and it must be
noted that strtol accepts leading whitespace which may or may not be
wanted. I've implemented what I believe to be a correct converter using
strtol which checks for the leading whitespace in [0], but it comes with no
guarantees as it's quite tricky indeed. The new C++11 wrapper std::stol and
friends in [1] may be of use here, but unfortunately they only have a
throwing interface.

All converters are added to the main boost namespace which is crowded
enough as it is -- hi boost::_1 --, and libraries should add their
functionality to their own namespaces in my opinion. With the current
design it's not possible to add them in the boost::convert namespace but I
believe they should be in their own namespace, may I suggest
boost::converter.

It must be said that the first two comments are minor at best, the last
regarding the namespace is more important but potentially quite trivial to
fix.

-- What is your evaluation of the documentation?

I've found it to be very hard to write documentation as the library author
as at some point you're so intimately familiar with your library that it's
hard to get a feel for what the user needs. Whilst there are some rough
edges I think the current documentation is decent enough for it not to pose
a problem. Again a few comments:

The motivation should probably be addressed first, not near the end.

The printf and std::strtol based converters aren't documented, and the
converters should probably be grouped into a sub-section along with an
explanation of how to write your own.

Last of all I'd like to see an examples directory showing the examples from
the documentation, you already have a test/encryption.hpp which probably
belongs there.

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

In the previous review I've argued the library its functionality should be
added to boost::lexical_cast. Whilst I still hold that position for the
previous version of the library I believe this is no longer the case for
the present version, largely due to the added support for multiple
backends. There would be no practical way for lexical_cast to add this, and
I believe it to be very useful indeed.

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

I've tried the library with Clang 3.2 and 3.4, and GCC 4.6 and 4.8 on Mac
OS X 10.9.3 and the examples from the documentation using the sstream
converter compile and run fine.

When I tried the printf_converter something as simple as:

    #include <boost/convert/api.hpp>
    #include <boost/convert/converter/printf.hpp>

    int main(int, char *[])
    {
       return boost::convert<int>::from("23",
boost::printf_converter()).value();
    }

failed to compile. It's missing includes as adding the following to
converter/printf.hpp fixes the problem:

    #include <boost/mpl/find.hpp>
    #include <boost/mpl/vector.hpp>

This probably indicates that you're trying to test too much in a single
file, and had the includes included via a different boost library.

In the previous review I noticed that boost::convert<int>::from('1',
cnv).value() spew a rather long error message. This has since been fixed,
the error message is now considerably shorter making it easier -- will it
ever be easy with C++ -- to figure out what's going on. I must note here
that the error message the latest versions of GCC produced is actually
easier to figure out than the one Clang produced.

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

Something between a quick reading and an in-depth study, now probably
leaning towards the latter.

-- Are you knowledgeable about the problem domain?

As the author of boost::coerce, which I've admittedly ignored for far too
long, I'm familiar with the conversion domain. Consequently I've seen the
pitfalls and endured the hardships in designing and implementing such a
library, my utmost respect for having it reviewed not once, but twice!

And finally:

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

In my previous review this was a straight no, stating that the
functionality should be added to boost::lexical_cast. With the current
design supporting multiple backends this no longer holds true as I've
stated in the evaluation of the potential usefulness of the library.

Again, the library is quite useful, but, I so strongly disagree with the
current user-facing interface that I can't straight-out accept it. Once the
library is accepted this interface is set in stone and like marriage it's
"speak now or forever hold your peace".

As such I'm voting YES, CONDITIONALLY on the interface being simplified for
the seemingly simple cases as most of my other comments were minor or quite
trivial to fix.

With that out of the way I'd like to propose the following if you're
interested. Let's have a look at whether it may be possible to merge the
functionally of boost::convert and boost::coerce, as they seem to have
grown towards each other with the possibility to support multiple backends.
Two years ago I'd never have proposed this, but I've mellowed a bit since
then.

You seem to have focussed on backends primarily backed by std::stringstream
and friends whereas I have focussed on a backend backed by boost::spirit.
These may end up complementing leading to a single library capable of
supporting both, being more generic and powerful to the user when done
right.

Regardless of whether we end up going this way I'd love to have a
discussion on the interface, either on the mailing list or offline as this
is currently a major stumbling block for me.

Jeroen

[0]
https://github.com/VeXocide/coerce/blob/master/libs/coerce/example/backend.cpp
[1] http://en.cppreference.com/w/cpp/string/basic_string/stol
[2] https://github.com/VeXocide/coerce/
[3] http://vexocide.org/coerce/