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

Subject: Re: [boost] Boost review of the Convert library is ongoing
From: Edward Diener (eldiener_at_[hidden])
Date: 2014-05-15 23:28:59

On 5/15/2014 9:42 PM, Vladimir Batov wrote:
> alex <alexhighviz <at> hotmail.com> 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<class type_in, class type_out, class cnv>
> do_something(type_in in, type_out out)
> { ...
>      convert<type_out>::from<type_in>(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<typename StringIn, typename ValueOut>
>>      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<typename StringOut, typename TypeIn>
>>      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<typename TypeOut, typename T, typename A>
>>      bool operator()(const std::basic_string<Char, T, A>& 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.