Re: [boost] [review] Convert library

Subject: Re: [boost] [review] Convert library
From: Roland Bock (rbock_at_[hidden])
Date: 2014-05-25 08:47:35

Hi,

On 2014-05-25 03:40, Vladimir Batov wrote:
>> 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");
> Here we go again... :-) lexical_cast will be haunting me to my grave. :-) 
>
No need for that :-)

First of all, I think I would not start each and every section of the
documentation with how to do something with lexical_cast...

Apart from that:
I think the main objection here is the amount of code you have to write
in order to get the most simple case: I have a value of type X and want
to convert it to a value of type Y.

I think it would be relatively simple to get to a much leaner interface.
For instance,

  * convert could have a converter template parameter, defaulting to the
    stringstream converter
  * convert::from() could have an overload that just takes the "from"
    parameter and provides the default-constructed converter itself.
  * convert could have a conversion operator for the OutType, which does
    the same as convert::value()

Then I would have

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

As far as I can see, conversion errors are handled by

 1. throwing exception (and letting the developer handle that where and
    how appropriate)
 2. using a default value
 3. calling a function (e.g. report the problem and provide a default
    value or throw an exception)

With 1) being the default, I could have overloads for 2) and 3), e.g.

int i = boost::convert<int>::from("17", 0);
int i = boost::convert<int>::from("17", []{
   std::cerr << "conversion problem, using default" << std::endl;
   return 0;} );

And if I want to use a specific converter that, I could use something like

MyConverter cnv{some args};
int i = boost::convert<int>::with(cnv).from("17", 0);

Or, if the converter can be default constructed:

int i = boost::convert<int, MyConverter>::from("17", 0);

In a similar way you could create the functors, for instance to be used
by algorithms:

auto f = boost::convert<int>::functor();
// no need for the <string> template argument if the call operator
// is a template, I'd say

Without having looked at the code that, I assume that an API like that
would be rather easy to accomplish.

I am not sure if my opinion is strong enough to be counted as a review,
but here it comes anyway:

> What is your evaluation of the design?/
I am not too fond of the frontend API as is. There are several
suggestions for a leaner API in the reviews. I second Julian's opinion
that it would be beneficial to mention parameters in the order [from,
to] instead of [to, from].

> What is your evaluation of the implementation?

The few files I looked at seemed ok, but I did not really do a code review.

> What is your evaluation of the documentation? 

Not being familiar with boost::lexical cast, I had difficulties finding
the things which are really about boost::convert (the note in "getting
started") does not help...

The documentation also leaves a lot to guesswork, IMHO. For instance,
the examples suggest, that convert<int>::result has a conversion
operator to bool which returns true, if the conversion was successful,
and false otherwise. If references to lexical_cast were omitted, there
would be enough room for explanations.

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

Personally, I have to admit, that I do not really see myself using the
library as is. I would write wrappers around it to get an interface
closer to what I want. But then, I have to wonder why I don't use the
converters directly? And since the converters are mere wrappers to
existing stuff afaict, I would probably use the underlying things directly.

So unless the API get's cleaned up in a way that does not make me want
to wrap it, I see no real use.

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

No.

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

About 3hours including writing this mail. Feel free not to count this as
a full review.

> Are you knowledgeable about the problem domain? 

I'd say yes, having written a few converters myself, having used quite a
few more.

And finally:

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

I try to avoid conversions like cats try to avoid water. The library
does not make we want to go swimming, so to speak.

Yes (since I need to swim sometimes), under the condition that the API
and the documentation get cleaned up, especially

  * parameters to -> from
  * default parameter for the converter
  * ability for the user of the library to provide methods to be called
    in case of conversion error
      o non-throwing methods would return a default value
  * Consider convert to be of a value on its own, not just in comparison
    to lexical_cast. If I need be annoyed by lexical_cast in order to
    understand the value of convert, there is something wrong...
  * Actually explain the library instead of just giving examples and
    letting the reader guess.

Regards,

Roland