Re: [boost] Boost review of the Convert library is ongoing

On 5/15/2014 9:42 PM, Vladimir Batov wrote:

alex writes:

...

This is a review from an enthusiastic C++ and Boost user, but relative outsider.

Alex,

You've made quite a few suggestions. It'll take me some time to get through them all and try and evaluate and address them. So it's just a quick acknowledgement reply to say thank-you for your interest and input. It's very much appreciated. I certainly agree that in many respect the documentation is not sufficient and the implementation is far from perfect. At this point I have to be realistic -- my overall effort is measured against (and tempered by) the real possibility of all of it wasted (if the review fails). Consequently, this submission is more of a proof-of-the-concept. If the community finds that the overarching design is sound and sufficiently flexible, then I'll have an incentive ;-) and an obligation to bring all the relevant components (the docs, the code) up to the standards... based on the agreed/approved design and behind the approved api (obviously, if the submission is successful).

Please find my further replies below.

V.

...

I would use his library because it provides a simple way to convert between strings and values. I particularly like that it returns an optional value giving the user a convenient way of dealing with failing conversions. Also, it is very useful to be able to apply the modifiers familiar from stringstream.

I found the documentation quite lacking from a user's perspective. The enormous emphasis on boost::lexical_cast is a distraction and a pain for new users. This should be a simple library and its understanding should not depend on being familiar with alternative libraries. You could have a separate page discussing the differences between Convert and boost::lexical_cast.

During our first (failed) "convert" review quite a few years ago many questions were "why not use lex_cast, why not do as lex_cast does?, etc". So to avoid answering the same questions over and over again I added lex_cast in the docs.

...

In my opinion the documentation should start by explaining how to convert between strings and numeric values. Next it could tell how to deal with failed conversions. Finally it could explain how to deal with stringstream modifiers.

The documentation doesn't seem to say anything about conversion to strings. I believe that is possible too? Conversions from strings to strings seems to be not supported, is that on purpose? For instance conversion from const char* to std::string would not work it seems to me.

String-to-type is obviously possible and there is quite a bit to be done improving the docs and the code... after/if it's decided it's worth doing.

...

The documentation doesn't seem to say anything about the from(converter) member function that seems to return a unary function object.

I believe you are referring the function which is used with algorithms. I'll see what I can say/add/do about it. Thank you.

...

I found it confusing that the function "from" is used for conversion; I would expect there to be an equivalent "to" that does the reverse of "from", but there is not.

I personally did not see the need for convert::to. So I do not have it. What would be a use-case where you might need convert::to and convert ::from would not do?

...

I would therefore prefer "get", "do","parse" or "convert".

Well, to me "convert<int>::from<string>" seems like a good "translation" from English "convert int to string" to C++.

I think you just misspoke here Vladimir, and meant "convert to int from string".

I do not immediately see mentioned alternatives doing it so succinctly. That said, I am all willing to listen and learn if you provide usage/deployment examples/comparisons.

...

The library says it will start using tr1::optional at some stage. Why not use boost::optional already now? It would make things a lot simpler and clearer.

Because right now boost::optional does not have all that is needed. Andrzej is working on extending boost::optional and I'll use it as soon as it's available.

...

The following usage pattern seems indirect and verbose:

boost::stringstream_converter cnv boost::converter<int>::result v1 = boost::converter<int>::from(str, cnv); int v2 = boost::convert<int>::from(str, cnv).value(); //may throw int v3 = boost::convert<int>::from(str, cnv).value_or(-1);

Would it not be easier to use: boost::stringstream_converter cnv; boost::optional<int> v1 = cnv.get<int>(str); int v2 = cnv.get<int>(str).get(); // doesn't throw int v3 = cnv.get<int>(str).get_value_or(-1);

Or, using Koenig Lookup to get rid of all the namespaces:

boost::stringstream_converter cnv; boost::optional<int> v1 = convert<int>(str, cnv); int v2 = convert<int>(str, cnv).get(); // doesn't throw int v3 = convert<int>(str, cnv).get_value_or(-1);

Well, here (I feel) you are suggesting quite a different design of using converters directly rather than via agreed interface. IMO that lacks the user-provider contract that 1) advertizes to the user what to expect (regardless of the converter used) and 2) forces the converter writer to follow the contract. Without these restrictions converter implementations are destined to diverge, provide varying api, etc. So, the pluggability and replaceability qualities (important to me and for a large-scale development) are no more.

...

Would it make sense to define the operator << for the boost::stringstream_converter? It would make it possible to pass modifiers in the same way as with stringstream.

Indeed, I had that interface during the first review (3 or 4 years ago). The interface kinda makes sense when the stringstream-based converter is looked at in isolation. If there is a strong push for this interface, I'll add it. No drama.

...

I think the effort to present Convert as a general conversion library is not helpful.

Here I have to respectfully disagree. My conversion needs do go beyond str-to-int and the like... not as often but still. So having a consistent API to do unknown-at-this-moment (i.e. generic) type-to-type conversions is important to me. My immediate application is MBCS strings to UCS strings.

...

A major strength and interest of this library seems to me that it responds to the same locale and formatting modifiers as stringstream.

IMO the major strength is that one can choose converter/functionality/performance on as-needed basis. stringstream-based converter is just one of many. Important for some, too slow and bloated for others.

...

This is not something offered generically by all proposed converters,

Correct... at this point. There is nothing stopping anyone to extend the existing (explicitly labeled as prof-of-concept) or write new converters supporting locale, etc.

...

and therefore by presenting Convert as a generic conversion library, you are reducing the expectations one can have of the library. It is not really clear to me what the attraction of a 'general conversion library' is: a generic interface for a unary function object where the user specifies the input and return type?

To me the importance of Convert is in providing one interface to varying conversion facilities... rather than any particular converter in isolation. IMO one *generic* interface (to do conversions or anything else) is essential if one writes any *generic* code:

template do_something(type_in in, type_out out) { ... convert::from(in, cnv); }

...

What has that to do with string-to-string encryption as suggested on the first page of the documentation?

String-to-string encryption is just one example of generic type-to-type conversion/transformation that the library can be deployed for.

...

I vote for inclusion in Boost, conditional to using boost::optional as a return type and a simplification of the interface. I would recommend dropping the idea of a pluggable general conversion tool and providing only the stringstream based conversion.

Well, I appreciate your vote for inclusion but I feel you have to reconsider either your vote or your evaluation of the *existing* design. By "simplification of the interface" and "dropping ...a pluggable" you are effectively suggesting/requesting a totally different overall design and API. To me it kinda sounds like "I vote for an autobahn but only if we build a railroad instead". So, first, you vote the autobahn down and then provide the specs for and vote on the railroad.

...

...

What is your evaluation of the design?

Good. Can be a bit slimmer and should make use of boost::optional.

I feel like I am being a pain in the neck but I am honestly confused. A pluggable converter seems to be the center-piece of the design that you do not seem to agree with.

...

...

What is your evaluation of the implementation?

I can't really judge, but looking at the code it seems some unused parts need to be cut out. Also, the operator() in basic_stringstream_converter seems overly complicated. Would it be an option to completely get rid of detail/string.hpp and use plain overloads? See suggested code below.

...

What is your evaluation of the documentation?

Poor, preoccupied by lexical_cast. Fails to document key functionality.

...

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

Good

...

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

Yes, VS2013, no problems at all.

...

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

I read the documentation, and had to revert to the source where the documentation was lacking. Studied some code that looked overly complicated in more detail.

...

Are you knowledgeable about the problem domain?

Hardly, but have had to convert between numbers and strings in the past and also had to deal with locale related problems. So would have liked to use this library.

...

And finally:

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

Yes. Conditional to using boost::optional as a return type and a simplification of the interface.

The interface might be perceived as complex due to it trying to cater different needs and usage patterns. My hope is that if one has one particular usage pattern (say, interested only in std::sstream-based convertor), then that generic interface can be wrapped up in a simpler custom api.

...

...

As always whether you do or do not feel that the library should be accepted into Boost please specify any changes you would like to see for the library to be better in your estimation.

Suggested code to reduce complexity of overloads in basic_stringstream_converter

template bool from_string(const StringIn& value_in, ValueOut& result_out) const { stream_.clear(); // Clear the flags stream_.str(value_in); // Set the content of the stream

return !(stream_ >> result_out).fail(); }

template bool operator()(TypeIn const& value_in, StringOut& result_out) const { stream_.clear(); // Clear the flags stream_.str(StringOut()); // Clear/empty the content of the stream

return !(stream_ << value_in).fail() ? (result_out = stream_.str(), true) : false; }

template bool operator()(const std::basic_string& value_in, TypeOut& result_out) const { return from_string(value_in, result_out); }

template<typename TypeOut> bool operator()(const char_type* value_in, TypeOut& result_out) const { return from_string(value_in, result_out); }

I'll see if and how I can incorporate your suggestions. Thank you.