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.
...
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
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". ;-) 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. :-)