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