Hi,
Having slept on this last night, and with a fresh mug of coffee in hand let
me see if I can clarify some things below.
On 25 May 2014 03:40, Vladimir Batov
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? :-)
My first reaction was that it was hard to say, and only time would tell. On second thought however I think you should not only have waited a little longer, but gotten a few real world users. It's very hard to argue against three to five real world users who are using your code in production stating they want it in boost. At that point most of the obvious implementation bugs should have been ironed out and they're convinced of your interface to the point of actually using it. That's testament! That said I'm well aware this isn't easy, I was delighted to have boost::coerce reviewed in [4] without my input. At times a mail would also pop up complaining my library stopped working in the latest version of, surprise, MSVC. I have no idea who's using it though unfortunately. 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. :-(
I've been meaning to put the finishing touches on it and get it reviewed but then life happened, and as I'm sure you're perfectly aware by now these reviews are no walk in the park.
-- 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).
This is a fair point but as other have argued boost::lexical_cast is used without the try-catch in a lot of places. Simply expecting it not to throw and dropping everything on the ground when it does isn't the nicest way to go about things but it does get things done. I guess the question is whether we should make it harder to use the library in such a way, and I think we shouldn't. If a users wants to potentially blow off a foot that's their call.
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);
Here I disagree but this is a matter of taste. Let me propose the following, a customisation point where users can choose their default converter within their application, defaulting to the lexical_cast_converter. This prevents people from having to instantiate a converter everywhere, either explicitly or inline, and it gives them the option to switch the converter for their whole application in a single place.
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.
Let me respond to this below.
-- 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? :-)
As said most of it was minor at best, and I wholeheartedly agree nailing the interface is the most important at this point. It worries me a little that the converters were put together in haste, these things shouldn't be rushed.
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.
That's true, however such a major revision should not be required within the first years of inclusion.
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.
Let me first say that I probably worded the "so strongly disagree" too harshly, and note that I didn't vote against the library, just the current interface. This is getting to the meat of the discussion though and looking back there are two distinct points I'm trying to make. First of all I think there should be a default converter, not requiring people to explicitly construct it everywhere. Above I proposed a customisation point where people can select the default converter for their project, defaulting to the boost::lexical_cast based converter. This feels like a fair middle-ground to me offering a more powerful interface to the user, and we can have the bike shed discussion on which converter it should default to. The lexical_cast one makes sense to me. Secondly I'm not a big fan of the .value() and .value_or(-1) interface but I can live with this. Especially since the std::optional also has the operator* and operator-> allowing int i = *boost::convert<int>::from("23"); which will make sense when people become more accustomed to std::optional. Note that for this to be efficient I'd like to see the optional to be movable, which isn't yet the case with boost::optional if I'm not mistaken.
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
With that out of the way I'd like to propose the following if you're 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.
As said above that was worded unnecessarily harsh, and wasn't mean that way.
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?
Exactly, once we nail the interface this is where we should be heading. Since you mentioned rushing the converters I suspect they may need a little love, and I can definitely integrate boost::coerce into boost::convert. I should expand on this as I don't mean simply adding a converter which calls coerce but fully porting over its functionality to convert. At some point I argued that coerce its functionality may be added to boost::spirit, but this would be a better home.
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.
I'm slowly warming up to the optional-based interface as it eliminates the throwing / non-throwing interface discussion, which is a good thing. Once again two comments, first of which is what's going to happen until the std::optional (whether in TR2 or elsewhere) is widely accepted. The optional provided by boost has a slightly different interface and isn't movable. The second is performance, with boost::coerce a lot of time was invested to prevent the interface from slowing down the conversion itself. I simply don't know how this interface will behave in that regard, when I have more time I'll try some benchmarking. Jeroen [4] http://www.kumobius.com/2013/08/c-string-to-int/