Re: [boost] [convert] Review

Subject: Re: [boost] [convert] Review
From: Vladimir Batov (vbatov_at_[hidden])
Date: 2011-04-28 17:54:30

> Thomas Heller <thom.heller <at> googlemail.com> writes:
> ...
> 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: ...
> Reading on ...
> Moving on ...
> So, after reading that I am thinking: "What is going on here? How do
> these different result types relate to each other?". 

I snipped your description of increasing complexity in the documentation for
brevity. I do understand your concern. The documentation style is quite standard
-- increasing/revealing more complexity as the user becomes more familiar with
the library. Still, I agree that considerable improvements can be made to the
documentation content. Those doc.-related improvements are inevitable if the
submission is accepted. 

> ... it doesn't even seem to work in every case, see failures
> that Jerone and Artyom got during their review.

Well, I am not surprised/concerned that it does not "work in every case".
Nothing ever does out of the box. I do not see it as a show-stopper or a
sufficient reason to reject a submission outright. That's what we have reviews
for, right? Accepting a submission with changes or a TODO list is quite a common
and reasonable practice.

> Another technical disadvantage of this design is that its really not
> extendable.

That was not my impression of the library. In fact, I tried to paid special
attention to make sure we get an extendable/specializable framework.

> 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
>    {};
> };

I am not sure I 100% follow your example but I'll try my best interpreting. ;-)

1. More often than not you do *not* need to write convert<foo>. The provided
'convert' is a template which most likely covers convert<foo>.

2. No, you do not need "to list every possible source type" there. That's what
templates and genetic programming greatly help with.

3. You do have to write a converter though (if that converter has not been
written already).

Please have a look at the current implementation of string-to-type conversions.
There  

1) we do not have convert<string>;
2) we did not have "to list every possible source type" there;
3) we did have to write an actual converter to do the job.

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

Uhm, why exactly the above is "not allowed"? Please have a look at the
submission code which has string-to-type generic implementation and then
string-to-bool specialized implementation. The string_to_bool.hpp has
exactly what you wrote above:

template <String> struct convert<bool>::converter<String>

> Its not really documented how a converter will look like. 

That's a fair point. I added the converter-related chapter at the very last
moment and did not really explain *how* converters are to be written. I'll put
that onto my TODO list and will address that if you change your 'no' vote to
'accept with changes'. ;-)

> And you don't mention if i couldn't
> use the operator<< and operator>> extension mechanism.

Well, that mechanism is only for string-to-type and type-to-string conversions
(as with lexical_cast) and I believe I do mention that in the documentation.
Namely, in the "Requirements on the Argument and Result Types" chapter: 
"TypeOut is InputStreamable with a std::istream& operator>>(std::istream&,
TypeOut&) defined". I tend to agree though that I'll need to elaborate that
further for clarity.

> The usefulness and completeness of your library is out of question.

Still, you voted 'no'. If it is as you indicate above, does it mean you voted
'no' to library's current state of documentation rather than to the submission
as a whole? If it is so, than you probably should have voted to "accept with
changes". Please do not get me wrong. I am not pushing you one way or another.
It is just that voters' opinions need to be stated clearly for the Review
Manager to ultimately count them for or against the submission.

> What i was criticising is the impression you get after reading the
> docs. Especially the reference section.

Criticizing is normal and healthy. I do it all the time. ;-) The problem is that
if you vote 'no', then you show a fundamental flaw/problem why the submission
is to be rejected and that's the end of it. If you later in the discussion come
up with suggestions to improve the implementation or documentation (which the
bulk of your criticism has been directed so far), then you should have probably
voted "accept with changes" as there is no point discussing and improving
something that is ultimately rejected/discarded. Does it make sense?

Best,
V.