Dear Edward, dear Vladimir, dear list, I am excited to submit my first official review for Boost. I hope many reviews will follow. :-) In order to be free of any biases, I have not read the other reviews (yet). I'm sorry to repeat anything that has already been discussed. Edward Diener wrote:
Comments, questions, and reviews will all be welcome for the library. Please try to sum up your review by answering these questions:
Are you knowledgeable about the problem domain?
A bit. I'm sure I haven't done as much with conversion as Vladimir, but I do know a little bit about it.
What is your evaluation of the documentation?
The first sentence of the introduction explains the primary motivation for the library. That's a good thing. Unfortunately, that sentence is too long and complicated; it discourages further reading. I would recommend splitting it into several shorter sentences, for example like this: "The main purpose of the framework is to provide a single consistent interface to various conversion methods. A single interface with uniform behaviour helps to improve productivity and correctness in software development." There is too much mumbo-jumbo: "The API facade specifies and formalizes the contract between the user and actual conversion facilities", and so on and so forth. This may give some users the impression that the library is only intended for advanced, specialised users. I presume this was not intended. At the bottom of the getting started page: "By design this is boost::lexical_cast's only behavior. Fairly straightforward and comprehensible. Unfortunately, it makes quite a few legitimate process/program flows difficult and awkward to implement. That is the niche that Boost.Convert is trying to fill." To me this seems like downgrading the purpose of your library. First you set out to provide a universal, extensible interface for anything related to conversion. Now, at second thought, your intention is to just eliminate the cumbersome exception handling of boost::lexical_cast. This is not the best way to sell your library to somebody like me. At the top of the performance page: "The C++11 family of std::to_string(), std::stoi(), etc. were not tested due to unavailablility." I'm OK with this for now, but I would be annoyed if I found this in the docs of the final release. Maybe there's some serious availability issue that I'm unaware of, but it still looks *lazy*. I would suggest either fixing the issue or not mentioning it. Overall the documentation does explain the library very well. I understood everything at first reading and it's clear why the library works the way it does. The succession of the chapters is natural. I'm also relieved that the documentation isn't overly lengthy. I did not find the reference section particularly enlightening, it's just a slimmed down copy of the code with some unexplained annotations. The motivation section is good, it provides both a more detailed motivation and a convincing use case. I would however suggest renaming it to "rationale" and moving it more to the front of the doc.
What is your evaluation of the design?
I like how the conversion API is completely type-agnostic, so that source type and target type can vary independently and neither needs to be a string type. I find the syntax `convert<Target>::from(source)` a bit counter-intuitive. A syntax in which the source comes before the target would seem more natural to me, but I understand that might be harder to implement. I strongly dislike the need to create a converter object (the ubiquitous cnv in the docs) and the need to pass it (of all places!) to the convert::from static member function. Having read the examples and the performance page I understand the potential benefit of using a persistent converter object that may accept additional arguments, but maybe you can involve the converter object elsewhere in the boost::convert interface. So instead of boost::cstringstream_converter cnv; boost::convert<Target>::from(source, cnv); I would like to see something more like boost::cstringstream_converter cnv; boost::convert<Target>::with(cnv).from(source); I would like to add to this that "convert" here rather means "extract". Code should be as self-explanatory as possible. I do definitely like how convert::result (to be replaced by tr1::optional) defers the decision of how to handle errors until later. I also like the `if (!result)` idiom, I think this an elegant way to handle conversion errors. At a higher level, I think the division of the library in an API facade and an extensible collection of particular converters is sensible.
What is your evaluation of the implementation?
The header converter/base.hpp defines a few macros, I think the names of those macros should be prefixed with BOOST_. I didn't spot any other issues, but I didn't try very hard either. The code looks quite clean to me.
What is your evaluation of the potential usefulness of the library?
I think a library like this is justified for special use cases that require lots of conversions, like XML processing as mentioned in the motivation section of the doc. I don't think it's very fit for general use because of the slightly long-winded syntax. Users will often prefer other interfaces that work through a single, short call. I also don't think this idiom would be appropriate beyond type conversions, for example in encryption as suggested in the doc. Of course, encryption in a sense is a conversion, but most functions are ultimately conversions and I think people would agree that the idiom proposed here would be needlessly cumbersome for most of such "conversions". Still, I certainly think there are valid use cases and I also think the approach taken in this library is appropriate.
Did you try to use the library? With what compiler? Did you have any problems?
I successfully compiled the test program with Apple Clang 5.1 (based on 3.4). It ran successfully except for one error: test_convert.cpp(313): test 'double_eng == eng_expected' failed in function 'int main(int, const char **)' This stage of the review took longer than necessary, partly due to my incompetence (fatigue) and partly due to a lack of documentation/adoption of standard Boost practices as well as a somewhat unfortunate choice to give #included files a .cpp extension. I didn't try anything else.
How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I spent roughly two and a half hours carefully reading the documentation and about half an hour quickly glancing over the code.
And finally:
Do you think the library should be accepted as a Boost library?
Yes, but it isn't ready. At least three things need to happen: 1) Make the doc more accessible to read, by making shorter sentences and using fewer fancy words, especially on the first page. 2) At least try very hard to think of a more natural interface. I think the most pressing issue is the current practice of converting "from" a converter object. 3) Further boostify the library: use prefixes for the macros, automate test building with a jamfile, add example programs. Hope this helps! -Julian