Re: [boost] [convert] Review

Subject: Re: [boost] [convert] Review
From: Thomas Heller (thom.heller_at_[hidden])
Date: 2011-04-29 11:13:43

On Thursday, April 28, 2011 11:54:30 PM Vladimir Batov wrote:
> > 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.

As I said. I like the style of the docs ... unfortunately you quoted that one 
away ...

Trying to complete my review, i tried to compile your tests, which unfortunately 
do not even compile ... and fail at runtime (bad locale ... so installed the 
locale to further show my good will ... finally succeed) after fixing them ... 
see attached patch.

What I was doing was to try to show the hidden complexity behind that innocent 
looking function "convert<Target>::from<Source>". Which has some serious flaws 
that are inherently caused by what i was pointing out as bad design.
All derived from studying the documentation. Sadly, the docs repealed so much i 
didn't even want to take a look at the actual code :(
For the sake of completeness let me back up my above claims (which you sadly 
decided to just ignore) now not starting from the documentation perspective but 
with actually analyzing the code you are proposing:

The prototype looks something like this (everything simplified without the loss 
of generality):
template<typename Target, typename Source, typename Fallback>
convert<Target>::converter<Source> convert<Source>::from<Target>(Target const& 
target, Fallback const & fallback)

convert<Target>::converter<Source> is implicitly convertible to:
1) Target
2) conversion::result<Target>

Furthermore, conversion::result<Target> is implicitly convertible to:
2) some safebool type

This is how you decided to design your library!
No, I don't have suggestion on how to fix this. As you noted below, this is a 
fundamental flaw in your design. And again, I didn't even have to look at how 
you implemented it.

And as you and others noticed lead to some strange, unexpected problems... which 
I believe can not be fixed that easily:
Consider attached the attached Point.cpp code.
The same behavior that Artyom noted in his review with the std streams. I don't 
think that you want to "fix" that in your library everytime a situation like 
that happens ...
Apart from the user confusion I mentioned in my earlier post. The "wtf" moment 
he gets because your function can magically return 3 different types, expect it 
can not...

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

Right ... this however is a real show stopper.

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

So ... lets take a closer at your claim that you provide an extendable powerful 
framework for type conversions (not only string to type and vice versa but 
actually anything to anything).
Short answer: Your claim is wrong.

Let us start small, by your first and actually documented mechanism to extend 
your library:
By overloading
1)
std::ostream & operator<<(std::ostream & os, Source const & s);
and
2)
std::istream & operator>>(std::istream & is, Target & t);

So far so good. There is nothing wrong with that. Excellent. With only this 
extension mechanism your library is just a "fixed" version of lexical_cast.
By having this mechanism you could theoretically provide everything you claimed 
your library can achieve (T -> iostream -> U). Except not.
You library (currently) only supports the conversion to and from strings. And 
there is no possibility to detect if the path T -> string -> U exists. At least 
not at compile time, which i would have expected to be possible for such a 
thing. Another design decision for which i actually needed to check the code.
You wouldn't have to go through all the hassle with this converter, result, 
implicit conversions etc.

Alright, lets move on to your "real" extension mechanism.
First, all bets are of for what has been said about the above extension 
mechanism. Cause well, i could decide to simply implement it completely 
differently which nullifies the whole effort you put into your library.
The other problem is of course the following:
"In an explicit specialization declaration for a member of a class template or a 
member template that appears in namespace scope, the member template and some of 
its enclosing class templates may remain unspecialized, except that the 
declaration shall not explicitly specialize a class member template if its 
enclosing class templates are not explicitly specialized as well. In such 
explicit specialization declaration, the keyword template followed by a 
template-parameter-list shall be provided instead of the template<> preceding 
the explicit specialization declaration of the member. The types of the 
template-parameters in the template-parameter-list shall be the same as those 
specified int he primary template definition. [14.7.3.18]
In your special case:
template <typename Target>
template <>
struct convert<Target, typename enable_if<...>::type>::converter<some_type>
{
   // ...
};

Is actually ill formed. Which restricts the possible extensions by a large set 
of types. Which is inherently caused by your design decisions.

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

Sure, makes sense. See above for my reasoning to why my vote remains "No".