Re: [boost] [review] Convert library

Subject: Re: [boost] [review] Convert library
From: Jeroen Habraken (vexocide_at_[hidden])
Date: 2014-05-25 06:34:33

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 <vb.mail.247_at_[hidden]> wrote:

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

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

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/