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.c... [1] http://en.cppreference.com/w/cpp/string/basic_string/stol [2] https://github.com/VeXocide/coerce/ [3] http://vexocide.org/coerce/