Andrzej Krzemienski
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
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?