Re: [boost] [convert] Review

Subject: Re: [boost] [convert] Review
From: Thomas Heller (thom.heller_at_[hidden])
Date: 2011-04-27 05:14:22

On Wed, Apr 27, 2011 at 3:13 AM, Vladimir Batov <vbatov_at_[hidden]> wrote:
>> Thomas Heller <thom.heller <at> googlemail.com> writes:
>> What is your evaluation of the design?
>> I think the design is not good.
>
> Well, that's not good. :-) Care to elaborate? For clarity we'll need to
> distinguish design from API and/or implementation. I only bring it up because
> your review is largely/exclusively concerned with API.

Yes, mostly API specific. See below.

>> The use of operator>> feels misplaced.
>
> I hope I covered it in my prev. reply to Jeroen Habraken.

Yes.

>>... the rational behind the non-throwing version isn't very clear
>> to me. It feels like a step backwards: Weren't explicitly returned
>> error codes obsoleted by exceptions?
>
> I tried covering that topic throughout the documentation. In a nutshell as I
> indicated in the "Getting Started" section the rationale behind not-throwing is
> that quite often "a conversion failure is not exceptional enough to justify
> throwing an exception". The std::fstream::open() behavior might be another such
> example. But I am sure you've already read that "Getting Started" section. So,
> I am not sure if I managed to answer your question... unless it was a rhetorical
> question.
>
> Stroustrup, chapter 14.1 stated that the fundamental idea behind exceptions "is
> that a function that finds a problem it *cannot cope with* throws an exception".
> So, my reading of it (and other sources) is that exceptions are a somewhat "big
> gun" more appropriate applied to the high-level design.
>
>> I don't get how the design really is superior to boost::lexical_cast,
<snip>
>> ... why it couldn't been implemented as an extension to that library.
>
> I tried covering that in my prev. reply to Jeroen Habraken.

Yes.

>> I don't think that having everything hidden behind this static from
>> function is wise and should be rethought. Different functions, for
>> different purposes would be better suited.
>
> I am not sure if I follow. I thought you indicated that "the API is too
> convoluted". Now "everything hidden behind this static from() function" does not
> exactly sound convoluted. Indeed, you find it too small instead. Surely you are
> not saying that making API bigger will be less convoluted and easier for the
> user? I thought we had one purpose -- conversion -- served by one function --
> convert::from. What other purposes? How are you suggesting to extend the
> existing API? Unfortunately, I am not sure I am quite following what you are
> actually suggesting as you seem to be at peace with one-function lexical_cast
> API.

So, lets discuss this a little more. Let me add that I am probably not
a potential user of your library or of lexical cast. Every points I
make is purely from reading through the documentation:

Let me add one positive point here:
I think the documentation is very well structured and leads the reader
from the very basic usecases to the more advanced ones. Very well
done!
However, and this is what i dislike about the library, its all hidden
behind one single function, and this is what i consider bad design and
convolution of the API. Even though you tried to be minimal and
simplistic, the function is overloaded with too different (in concept)
behavior that its really confusing to get what the function is really
doing. So my end result is that its not really minimal and
minimalistic, but bloated ... sometimes, more is less.
To better understand my reasoning and my above rant, let's try to
pretend that we are absolute beginners, someone told us about your
library and we want to use it, so we start reading the docs:
Nice, this "int i = convert<int>::from(str);" does exactly what I want
and doesn't look to complicated! So, you got my interest, good! Let's
see what additional gimmicks your library has to offer!
Reading on I begin to realize that convert<T>::from does not really
return T but something strange that you call convert<T>::result. First
intention here: That doesn't look like what I really want! Reading on
... obviously that has something to do with the fallback and that the
conversion might fail ... First impression here: Why not use
boost::optional in the first place? The answer to that question has to
do with the operator abuse we discussed earlier.
Ok, i might live with that ... more on that later.
Moving on with the documentation the last section tells me that the
return type of convert<T>::from is not really convert<T>::result but
convert<Target>::converter<Source>.
So, after reading that I am thinking: "What is going on here? How do
these different result types relate to each other?". Apart from my
confusion it doesn't even seem to work in every case, see failures
that Jerone and Artyom got during their review.
Another technical disadvantage of this design is that its really not extendable.
Example:

// my cool new target type.
struct foo {};

template<>
struct convert<foo>
{
   // we need to list every possible source type here ...
   template <typename Source>
   struct converter
   {};
};

struct bar;

// not allowed:
template<>
struct convert<foo>::converter<bar>
{
};

Despit the fact that this is not allowed. Its not really documented
how a converter will look like. And you don't mention if i couldn't
use the operator<< and operator>> extension mechanism.

<snip>

>> After reading through the documentation I got the feeling that the
>> library is incomplete and not really finished yet.
>
> Well, I happen to disagree with your feeling. I suspect it is hard/impossible
> to qualify this or any library as complete. 'Convert' is more of a framework to
> incorporate more type conversions if/when needed and to explore other (like
> string-to-string) conversions if such a need arises. It does not mean the
> library is not usable now. In fact, I've been using it quite extensively and
> instead of lexical_cast for a few years.

The usefulness and completeness of your library is out of question.
What i was criticising is the impression you get after reading the
docs. Especially the reference section.
Its a matter of how you are trying to sell your product. Not whether
there are still features missing ... that is irrelevant and i don't
care about things I can't do, more about the things I can. Mostly
pointing at the section "More About Converters, Customization,
Optimization and Performance". This is kind of sad, cause it seems
like this looks like your ultimate selling point of the library.

>> Almost any feature is experimental.
>
> Well, that's certainly wrong (IMHO of course).

Ok, this was probably mostly my personal reception after finishing my
first read of the documentation.

>> Do you think the library should be accepted as a Boost library?
>> No. I second that it feels more like a extension to boost::lexical_cast
>> than anything else.
>
> Please see my reply to Jeroen Habraken.

Yes.