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

Subject: Re: [boost] [review] boost::convert
From: Vladimir Batov (vbatov_at_[hidden])
Date: 2011-04-26 18:47:09

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

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

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.

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

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

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

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. 

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

Best,
V.