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
> -- 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
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
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:
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
> > ... 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
> 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
std::ostream & operator<<(std::ostream & os, Source const & s);
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. [22.214.171.124]
In your special case:
template <typename Target>
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
> 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
> up with suggestions to improve the implementation or documentation (which the
> bulk of your criticism has been directed so far), then you should have
> 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".
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk