Re: [boost] [review] convert library

Subject: Re: [boost] [review] convert library
From: Vladimir Batov (Vladimir.Batov_at_[hidden])
Date: 2014-05-19 20:39:35

Julian,

Thank you for your submission and comments. They are much appreciated.

On 05/20/2014 08:20 AM, Julian Gonggrijp wrote:
> ...
>>     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...
>
> 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.

I understand. :-) I tried to squeeze what I saw as important to a few 
introductory sentences. Namely, the formalization how the user 
"interacts" with / deploys conversion facilities. thesaurus.com does not 
give me anything better to replace "formalization" with. :-)

> ...
> At the bottom of the getting started page:
> ...
> 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.
Thank you. If the submission is successful, I'll be re-working the 
implementation and the docs based on all the input received.

As for std::stoi(), then I am on Ubuntu 12.04 and gcc-4.6. Those 
facilities are not available to me that I, in fact, stated as "due to 
unavailability". ;-)

> ...
>
>>     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.
For type deduction purposes the order has to be
template<class TypeOut, class TypeIn, class Converter>
optional<TypeOut> convert(TypeIn, Converter)

If you could post a snippet how to achieve what you are suggesting, I'd 
love to study it and possibly incorporate.

> 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.

The interface and function names are very subjective subjects and 
everyone has a (differing) opinion. Unfortunately, it's impossible to 
satisfy them all. As a few reviewers expressed their dislike of "from" I 
am currently leaning towards

int v = boost::convert(str, cnv).value();

It's not as self-explanatory but more in line how lexical_cast does it.

As for passing converter to a separate function call, I suspect it'll 
have performance penalties as the converter will need to be stored 
somewhere for later use. I had similar design during the first "convert" 
review -- performance was shocking.

> 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.
I am really glad to hear that because IMO it's the most important design 
decision that I wanted evaluated and discussed.

> ...
>
>
>>     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.
Understood. Here I feel that the community is not as passionate as I am 
about the advantages of uniform and *familiar* interface. :-(
The problem is that to use that mentioned "short call" one has to 
know/remember it and how to use it. It's far from easy when one deals 
with many different libraries providing different interfaces.
> 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".
I hear you. Although I admit I do not share your view. My example would 
be lexical_cast. Coding a type to be used with lexical_cast is a pain. 
However, when I come across lexical_cast call (in an unfamiliar code), I 
immediately know what's going on and what the behavior and side effects 
and so on. You do not get that with some "int v = foo.to_int();", do you?

> ...
> 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 **)'
I believe that's because different compiles and platforms have different 
locale representations. Say, Microsoft's "1.235e-002" is "1.235e-02" 
with gcc. I wonder what was "double_eng" in your test (printed on line 310)?

> ...
>
>> 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.
>

Thank you for your "conditional yes" vote. It's much appreciated... and 
more things on my TODO list. :-)