Re: [boost] [review] boost::convert

Subject: Re: [boost] [review] boost::convert
From: Jeroen Habraken (vexocide_at_[hidden])
Date: 2011-04-27 13:14:19

On 27 April 2011 00:47, Vladimir Batov <vbatov_at_[hidden]> wrote:
>> Jeroen Habraken <vexocide <at> gmail.com> writes:
>> Hi,
>> My review:
>
> Jeroen,
>
> Thank you for your input. It's much appreciated. Please let me address/reply to
> a few points that you make if not in an attempt to change your mind ;-) but for
> the benefits of other readers (if there are such).
>
>> Although this might be a matter of taste I find the choice of
>> operator >> to add modifiers rather odd, it smells a bit like
>> operator abuse. Also, it forces you to use braces in
>> std::cout << (boost::convert<int>::from("12") >> std::hex) << "\n";
>> for example, maybe using a member function is an option.
>
> Yes, indeed operator>> is the matter of taste and it's not exactly my taste
> either. In fact, at one stage I had the (manipulator_ = std::hex) interface
> available to align with (locale_ = ...), etc. As nothing is carved in stone we
> might discuss bringing that interface back (if it is decided to proceed with the
> library of course). Still, after discussions we settled on operator>> as we felt
> it was complimentary to the standard std::stream-based manipulators and,
> therefore, immediately familiar to the potential user.

Do you have a link to this discussion (if it was on a mailing list),
I'm curious as to how this decision was reached?

>> In might be nice to add a way to return a default-constructed
>> object to the non-throwing interface. This is useful in combination
>> with boost::optional: boost::convert<boost::optional<int>
>> ::from_default("XXX") returning an boost::optional<int>() instead
>> of having to write
>> boost::convert<boost::optional<int> >::from("XXX",
>> boost::optional<int>());.
>
> First, I have to disagree on returning a default-constructed object. Here we are
> likely to differ in our interpretations/treatments of the default constructor.
> To me it's just another constructor which happens not to have any arguments. You
> seem to treat that constructor as some kind of special/default even though in
> general terms it might not be even available for a class.

I do indeed consider it to be a special case but I can see your point.

> Secondly, I admit I've never thought of the above-mentioned deployment like
> boost::convert<boost::optional<int>. In fact, I am still having difficulties
> coming up with reasonable use-cases for such a deployment. I thought
> convert<int>::result does what you are seemingly trying to achieve with
> boost::optional.

They seem to provide the same functionality but using
boost::convert<int>::result when you need a boost::optional<int> in
the end doesn't make sense. Can boost::convert<>::result be replaced
by boost::optional?

>> The documentation covers all aspects of the library quite nicely.
>> This in contrast to the reference which seems mostly empty, in my
>> opinion it doesn't add much and can simply be removed.
>
> Yes, I readily agree that the Reference section is not perfect. ;-) I've been
> struggling with Doxygen deployment as part of QuickBook processing. That is on
> my TODO list (if it is decided to proceed with the library of course).
>
>> Since the behaviour of convert seems to be different to lexical_cast
>> (see below) it might be useful to add a section listing these
>> differences.
>> Lastly the documentation contains a couple of examples, it would be
>> nice to have these as separate files in libs/convert/example.
>
> Both of your suggestions make sense to me and I'll look into addressing them
> (the disclaimer again - if it is decided to proceed with the library).

Great.

>> boost::convert<int>::from("-123.2") behaves differently from
>> boost::lexical_cast<int>("-123.2"), the former returns -123 whilst
>> the latter throws. Whilst I'm not saying one is better than the other,
>> the differences should be documented for people switching.
>
> I feel that lexical_cast behavior is correct. I'll look into addressing the
> issue (the disclaimer again - if it is decided to proceed with the library).
>
>> boost::convert<int>::from('1') results in a rather long error
>> message ... this should be made clear in a more
>> user-friendly way.
>
> Yes, haven't I noticed? ;-) That problem is not exactly the convert lib problem
> and I believe is being addressed by C++0x. Still, I'll look into addressing the
> issue (the disclaimer again - if it is decided to proceed with the library).

Eric Niebler argued that bad template errors are a library bug (see
http://cpp-next.com/archive/2010/09/expressive-c-why-template-errors-suck-and-what-you-can-do-about-it/)
and I agree with him, this should be fixed.

>> Do you think the library should be accepted as a Boost library?
>> No it should not, instead the extra functionality should be added
>> to boost::lexical_cast.
>
> I see that you are quite firm in your opinion that "the extra functionality
> should be added to boost::lexical_cast". ;-) And you know what? That was my
> original request a few years back. Unfortunately, the cruel reality of life does
> not seem to give a hoot about what we think would be the best way to proceed.
> So, one can just sit there adamantly insisting on nothing less but his
> never-happening "ideal solution" or one can accept the reality and adapt to the
> circumstances. I needed a real solution to my problems. I had to implement them.
> As I understand "starting fresh" was a collective decision on this list quite
> some time back. That is the reason this convert thing is currently under the
> review. I again urge you to please dig the archives to see why that "extra
> functionality should be added to boost::lexical_cast" view is not exactly
> original and most importantly why it was decided *not* to proceed as you insist.

As you say this is a solution to your problem, not the problem that
boost::lexical_cast has which should ultimately be addressed by
someone. I'll have a look in the archives (do you happen to have a
link?).

> More so, after having to implement all that functionality let me assure you that
> I've come to a firm conclusion that that additional
> functionality/configurability is *not* implementable within the boundaries of
> the lexical_cast interface. In particular, the TypeIn and TypeOut types must be
> discriminated, i.e. one type has to be specialized first. Without going into
> details I'll just say that otherwise is just does not work. So, it has to be
> convert<TypeOut>::from<TypeIn>. You can take my word for it or you could walk
> that road yourself. Regardless of your decision (to believe me or try it out
> yourself) I feel it is quite short-sighted to deny the users functionality they
> need. I perfectly understand that this library is nothing revolutionary (like
> shared_ptr, bind, Spirit) but people seem to have need for that. Simply go to
> the Vault and see how many downloads of that convert has been so far compared to
> other libs.

I'm not saying your library doesn't solve a problem for users, it
definitely does. However, it isn't the optimal solution in my opinion.
Other approaches have been mentioned (see this thread for example,
http://lists.boost.org/Archives/boost/2005/04/84296.php) but were
rejected by the original author because he felt it should look like a
cast. Since he has abandoned the library I believe it is time for
someone to step up and take it to the next level. Anyways, we've had
this discussion before.

> As for lexical_cast "can not be deprecated or worse, removed", then I used to
> use lexical_cast a *lot*. I do not use it anymore and my world did not implode.
> The reason -- it does not do what I need it to do. Therefore, the impact of
> deprecating lexical_cast might not be as disastrous as you might imagine. Things
> are being deprecated every day -- vinyl records, magnetic tapes, CDs, (long list
> follows) -- it's called progress. ;-)

This analogy doesn't hold, a vinyl or CD can't be adapted to meet new
or extra demands, boost::lexical_cast on the other hand can.

> Best,
> V.

Regards,
Jeroen