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

Subject: Re: [boost] Boost review of the Convert library is ongoing
From: alex (alexhighviz_at_[hidden])
Date: 2014-05-15 12:47:28

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

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.

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.

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

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 would therefore prefer "get", "do","parse" or "convert".

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. 

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::converter<int>::from(str, cnv),value(); //may throw
    int v3 = boost::converter<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);

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.

I think the effort to present Convert as a general conversion library is not
helpful. A major strength and interest of this library seems to me that it
responds to the same locale and formatting modifiers as stringstream. This
is not something offered generically by all proposed converters, 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? What has that to do with string-to-string encryption
as suggested on the first page of the documentation?

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. 

>     What is your evaluation of the design?

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

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

>
>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);
    }