Re: [boost] [review] Convert library

Subject: Re: [boost] [review] Convert library
From: Vladimir Batov (vb.mail.247_at_[hidden])
Date: 2014-05-24 21:40:30

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.html
Sent from the Boost - Dev mailing list archive at Nabble.com.