[boost] Convert library -- Andrzej's review

Subject: [boost] Convert library -- Andrzej's review
From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2014-05-17 16:33:10

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

This is a good and useful library. This is what I needed for a long while:
a tool that allows me to convert a string to an int or float, but that
would not treat the failure to convert as something unusual, and that would
be locale-aware. Convert library offers this. It also offers other things
that I fail to appreciate, but I accept that it is supposed to satisfy more
expectations than just mine.

Your choice of the generic type converter:
  bool operator()(TypeIn const&, TypeOut&);
I think it is the right choice: this is the signature that does not impose
any run-time overhead. Unlike boost::convert<TypeOut>::result, which does
impose some run-time overhead of copying (or at least moving) the TypeOut.
But that's acceptable. If the conversion needs to be super fast, one can
fall back to using the converter directly.

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

I personally do not like this trick to force me to type this "from" in
"convert<int>::from". 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. But I do not
intend to argue over it. You wrote the (good) library, so you have the
privilege to pick the name.

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.

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.

Regards,
&rzej