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

Subject: Re: [boost] Boost review of the Convert library is ongoing
From: Vladimir Batov (vb.mail.247_at_[hidden])
Date: 2014-05-15 21:42:05

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