Jeroen, Thank you for your review it is very much appreciated. And, indeed, by your own admission you've "mellowed a bit since" a few years ago. Still, I see some bits left of "so strongly disagree"... should I have waited for a little bit longer? :-) Jeroen Habraken wrote
Hi,
As promised my review of the boost::convert library. Note that the questions were answered in a seemingly random order, thus I hope it all comes together. Secondly, being the author of the boost::coerce library [2][3] I may be a little biased at times, please forgive me.
Now that you mention "coerce" I do remember you mentioning it back then during review #1. I completely forgot about it and I still can't see it in Boost. Now when I look for "coerce" Google only gives 2012 links. :-(
-- What is your evaluation of the design?
There are two parts to this, the design of the backend and the design of the interface that the user predominantly sees. I have little to say on the first, the notion of multiple backend converters is a big step forwards and their interface is sane.
I'm far less convinced when it comes to the user-facing interface as I believe the simple tasks should be simple and intuitive, whereas the harder tasks may be harder.
The most trivial use of boost::convert, and boost::lexical_cast and boost::coerce simply don't compare in my opinion.
boost::cstringstream_converter cnv; int i = boost::convert <int> ::from("23", cnv).value();
versus
int i = boost::lexical_cast <int> ("23"); int j = boost::coerce::as <int> ("23");
Here we go again... :-) lexical_cast will be haunting me to my grave. :-) For starters, please stop using that silly lexical_cast deployment example as our benchmark. Such deployment does not exist. lexical_cast cannot possibly be deployed like that in any real application. The correct and responsible deployment is int i = -1; int j = -1; try { i = boost::lexical_cast<int>(str); } catch (...) {} try { j = boost::lexical_cast<int>(str); } catch (...) {} which is not exactly the two-liner that you sited! Compared to that the "convert" usage is boost::cstringstream_converter cnv; int i = boost::convert<int>(str, cnv).value_or(-1); int j = boost::convert<int>(str, cnv).value_or(-1); does look simple and intuitive enough IMO, configurable and potentially more efficient. (In the post_review branch the convert::from API has been retired).
When the convert library makes it into boost I'd want to mentally deprecate lexical_cast as a library I should use, and always use convert instead. This means convert should have a default converter, which may well be backed by lexical_cast with an interface such as:
int i = boost::convert <int> ::from("23").value();
This still doesn't completely satisfy me as I'd like to write
int i = boost::convert <int> ::from("23");
Again, I can't possibly stress more that the (deceivingly simple) usage examples you insist on do not exist in real applications -- they are functionally-incomplete (as they do not handle all possible use-cases). Still, with the post_review branch one can responsibly(!) write int i = boost::convert<int>(str, cnv).value_or(-1); I think I can live with that idea of using lexical_cast-based converter by default... However, I personally favor explicitness/readability and, therefore, int i = boost::convert<int>(str, boost::lexical_cast_converter()).value_or(-1);
but it'd be an improvement in my opinion. Now I'm aware lexical_cast doesn't have a non-throwing interface, and it probably never will, but I've solved the problem as follows in boost::coerce:
int i = boost::coerce::as_default <int> ("XXX"); int j = boost::coerce::as_default <int> ("XXX", tag::hex()); int k = boost::coerce::as_default <int> ("XXX", 23); int l = boost::coerce::as_default <int> ("XXX", tag::hex(), 23);
The former two returning the default-constructed int, 0,
1) For the first 2 deployments: I personally cannot justify the former two returning int(). The questionable perk of not typing a few more characters comes at the expense of introducing considerable ambiguity and potential misinterpretation. 2) For all 4 deployments: Again, no matter how temping it is to have a "simple" interface like that (similar to lexical_cast), it simply does not cut the mustard (so to speak) in real life... simply because it is not complete. Namely, it is non-deterministic: int k = boost::coerce::as_default<int>("-1", -1);
the latter two returning 23. That said, I'll be the first to admit the coerce interface isn't perfect, the fact that I need SFINAE to distinguish between as_default calls with two parameters, the second either being a tag (somewhat similar to your converters but not quite) or the default value goes to show just that. However I'd prefer
int i = boost::convert <int> ::from("23", cnv); int i = boost::convert <int> ::from_default("23", cnv, 0);
to the current interface using value_or(0). This is subjective though, and quite possibly a matter of taste.
The "convert" signature (I keep referring to the post_review branch) is actually dead simple: std::tr2::optional<TypeOut> convert(TypeIn const&, Converter const&); The rest, namely, value(), value_or(), throwing behavior come from std::tr2::optional. I personally find its interface very intuitive. I do understand that you insist on a different interface for "convert"... as in lexical_cast and "coerce"... however, as I stated in the docs, in real life that API makes quite a few legitimate process/program flows difficult and awkward to implement.
In summary, before this turns into too long a rant, I think the current user-facing interface can be improved substantially, but I'm well aware this is subjective.
Yes, I believe it is crucially important we come up with process-flow-complete or functionally-complete(!) API. So far, std::tr2::optional<TypeOut> convert(TypeIn const&, Converter const&); seems the best. I tried to explain why alternative examples you provided do not seem to always work.
-- What is your evaluation of the implementation? ...
Snipped for brevity. I agree with all mentioned points and will be addressing them if we decide to go ahead with "convert". In my defence it all was put together in haste (especially converters). What is *actually* important to me at this stage is nailing the "convert" interface which so far is std::tr2::optional<TypeOut> convert(TypeIn const&, Converter const&); If we decide to go ahead with "convert", I'll be addressing all comments so far to the best of my abilities. Putting converters into boost::converter namespace sounds like a good idea... and I am hoping that people with more specific knowledge step in and provide excellent converters catering for different purposes... like coerce-based? :-)
And finally:
-- Do you think the library should be accepted as a Boost library?
In my previous review this was a straight no, stating that the functionality should be added to boost::lexical_cast. With the current design supporting multiple backends this no longer holds true as I've stated in the evaluation of the potential usefulness of the library.
Again, the library is quite useful, but, I so strongly disagree with the current user-facing interface that I can't straight-out accept it. Once the library is accepted this interface is set in stone and like marriage it's "speak now or forever hold your peace".
As such I'm voting YES, CONDITIONALLY on the interface being simplified for the seemingly simple cases as most of my other comments were minor or quite trivial to fix.
Well, I personally do not believe "speak now or forever hold your peace" holds true and stops any potential improvements if/when necessary. Given your familiarity with Spirit I'd expect you to be aware of it going through major incompatible revisions over time. And Spirit is not an exception here. Here, again, IMO the "convert" interface is the center-piece of "convert" design. Converters' signature, et al. are secondary and can change without affecting users (much). The "convert" interface is crucial. So, if you "so strongly disagree" with it, you cannot possibly vote "yes"... even conditionally... especially when your "condition" is to abandon the existing design. :-) Having said that I feel it needs to be pointed out that saying "I do not like it" without providing a suitable alternative is not sufficient. Unfortunately, so far I have not seen any serious alternatives to the existing proposed "convert" API. Please do not get me wrong. Give us an interface covering all use-cases (hopefully mentioned in the docs). If community decides it's better, I'll incorporate it.
With that out of the way I'd like to propose the following if you're interested. Let's have a look at whether it may be possible to merge the functionally of boost::convert and boost::coerce, as they seem to have grown towards each other with the possibility to support multiple backends. Two years ago I'd never have proposed this, but I've mellowed a bit since then.
You seem to have focused on backends primarily backed by std::stringstream and friends whereas I have focused on a backend backed by boost::spirit. These may end up complementing leading to a single library capable of supporting both, being more generic and powerful to the user when done right.
Regardless of whether we end up going this way I'd love to have a discussion on the interface, either on the mailing list or offline as this is currently a major stumbling block for me.
Work together? Sure. Again, I actually see this proposal as an attempt to gather all input that I might incorporate for everyone's benefit. So, in this review I am not a "visionary" but the requirements gatherer and implementer. :-) For me it's not about ego but about having something community-accepted and standardized (I am a sucker for standardization and unification and homogenization :-)) That said, you might like to soften a bit your "so strongly disagree" stance. :-) Everyone brings their designs to the table. We pick the most functionally-complete. Nothing personal. What I see important at this stage is to settle on the "convert" user API. Currently, std::tr2::optional<TypeOut> convert(TypeIn const&, Converter const&); Then, I'd focus on std::sstream-based converter and you might bring coerce-based one for everyone's benefit. What do you think? The discussion of the converter signature would be close second on the list. I am resisting but inevitably drifting towards the signature Alex proposed: void operator()(TypeIn const&, std::tr2::optional<TypeOut>&); as it seems potentially more efficient and less restrictive than current bool operator()(TypeIn const&, TypeOut&); and my attempts to deploy std::tr2::optional<TypeOut> operator()(TypeIn const&); signature fail to cover all deployment cases so far. Your contributions are most welcome. -- View this message in context: http://boost.2283326.n4.nabble.com/review-Convert-library-tp4662821p4662827.... Sent from the Boost - Dev mailing list archive at Nabble.com.