[boost] [review] convert library

Subject: [boost] [review] convert library
From: Julian Gonggrijp (j.gonggrijp_at_[hidden])
Date: 2014-05-19 18:20:30

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