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