Re: [boost] Convert library -- Andrzej's review

Subject: Re: [boost] Convert library -- Andrzej's review
From: Vladimir Batov (vb.mail.247_at_[hidden])
Date: 2014-05-18 09:16:19

Andrzej Krzemienski <akrzemi1 <at> gmail.com> writes:

> This is the review of the Convert library. My vote is a CONDITIONAL YES:
> this library should be accepted into Boost subject to conditions described
> at the end of this message.
> ...

Andrzej, thank you for your review and your "CONDITIONAL YES" vote. It's
very much appreciated. As for "conditional", then that has always been my
goal and expectation -- this current submission is far from being ready for
inclusion as-is. If we decide to proceed down the path described in the
submission, I'll certainly be extending std::sstream-based converter first
(I use it most), etc. 
 
> I appreciate the stringstream-based converter most. About the ability to
> plug other converters, I am just not convinced. Why would I use other
> converters? I know, lexical_cast is supposed to be faster in some cases,
> but if I was interested in this performance I would have used lexical_cast
> directly. 

Yes, I understand your reasoning. I personally find the pluggability quality
to be quite important. Let me try and explain. First, from the first
submission review it was clear to me that not everyone was interested in the
std::sstream-based functionality and prepared to accept relative slowness of
std::sstream. The pluggability addresses that issue with minimal effort and
makes the library useful for much wider programming community. 

Secondly, the separation of the API/facade from the actual converter -- the
pluggability -- allows to write and deploy new/better/domain-specific
converters easily -- as long as those converters conform to the API-imposed
contract. The difference in "plugging a new converter" vs "replacing an
existing directly-used converter with a new converter" is considerable in a
large-scale development as the former only replaces a pluggable component
(leaving the "plumbing" intact) and the latter may create a lot of ripples
in the related code. An every-day example might be unplugging and replacing
an electrical device. Clearly, with no pluggability replacing such a device
(directly connected to your house wiring) might be quite a hassle. Yes, on
one-person-writing-new-code level that pluggability looks like a hassle. My
argument is that on the large-code-base, maintenance, changed-requirements
phase pluggability is the only sane way to manage that change.

> The alleged ability to use this library to change the encoding of
> the strings or to convert between "related" types like ptime and time_t and
> Time -- I am not convinced: do you expect such conversions to fail and to
> report such failure the same way as you report the impossibility of
> interpreting a string as a float?

Well, I am not convinced either. :-) Still, I used it for MBCS to UCS string
conversion in the past and did not see anything wrong with it. The main
reason was to use an already familiar API instead of the need of
inventing/learning/maintaining yet another new one.

> I personally do not like this trick to force me to type this "from" in
> "convert<int>::from". 

It's been a while and OTOH I do not remember what/how it was exactly but the
main purpose of "from" is to separate TypeIn from TypeOut. Not

template<class TypeOut, class TypeIn, class Converter>
optional<TypeOut> convert(TypeIn, Converter)

but 

template<class TypeOut>
struct convert
{
    static optional<TypeOut> from(TypeIn, Converter);
}

That way (at least I felt so back then) it was the only way to provide the
ability to *reliably* specialize separately on TypeIn and/or TypeOut. Again,
maybe that original decision/design is not valid or important and needs to
be revisited.

> If I was only passing my string as an argument, this
> would be fine, but when I am also passing the converter object, it reads
> that I am converting from both the converter and the string. 

Yes, I remember you suggesting defaulting the converter parameter to the
std::sstream-based and my initial intention to do just that. I then backed
off as I realized the create-converter-once-use it-many-times can be easily
lost. If we could come up with something like

boost::convert::use(some_converter); 

int v = convert<int>::from(str); // some_converter used

That might address your concern, right? Any crazy tricks to achieve that
without inheritance?

> ...
> 
> I am also not convinced about the potential of the 'facade' part. I can see
> that you can see the potential in it. But to me separating the library into
> the facade and the stringstream converter adds no practical value. But
> the stringstream
> converter itself is enough to stand for the usefulness of library.

I tried addressing that somewhat above -- API facade is the
consumer-provider contract. Based on that the consumer knows what to expect
and the provider knows what to provide *without consumer-provider
interaction*. In practical terms it is that every time I browse somebody
else's code and see the interface, I immediately know what it does without
reading the docs, learning new api, etc. In real life, when one is reading
(trying to fix) somebody else's code the "reading the docs, learning new
api" seems like a considerable diversion and often does not happen. So, the
understanding of the code quickly turns into a guessing game. So,
productivity is no more.

> My condition on the acceptance of the library is that its performance
> should be improved as indicated by others, in particular, provide move
> semantics for boost::convert<TypeOut>::result (on compilers with rvalue
> references). It is enough that you commit to doing it in the future.

My intention, in fact, is to ditch convert<>::result for boost::optional
given that you'll be pushing it into 1.56, right?