[convert] Review
Hi, Following is my review of the convert library proposed by Vladimir Batov.
- What is your evaluation of the design?
I think the design is not good. After having read through the documentation, I think that the API is too convoluted. The use of operator>> feels misplaced. From the documentation it is not really clear what the relation between the target type, the nested result and the converter function object is. Additionally, 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? Moreover, I don't get how the design really is superior to boost::lexical_cast, and why it couldn't been implemented as an extension to that library. The use of this converter function object looks like a nice concept at first sight but, as mentioned above, in the end, probably adds more confusion and problems than anything else. 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.
- What is your evaluation of the implementation?
Didn't take a look at the implementation.
- What is your evaluation of the documentation?
The overall documentation is Okayish. There are some minor typos and misplaced words. Proofreading it again should fix this. What is std::u8string? The reference section feels like a bad joke. It is incomplete and doesn't really add value. After reading through the documentation I got the feeling that the library is incomplete and not really finished yet. Almost any feature is experimental. One feature is mentioned but not really documented: The extension of the converter.
- What is your evaluation of the potential usefulness of the library?
There is no doubt that the features implemented in the library are useful.
- Did you try to use the library? With what compiler? Did you have any problems?
Didn't try to use it.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
Read through the documentation.
- Are you knowledgeable about the problem domain?
No.
And finally: 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. Best regards, Thomas
Thomas Heller <thom.heller <at> googlemail.com> writes: Hi, Following is my review of the convert library ...
Thomas, Thank you for your review. It's much appreciated.
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.
The use of operator>> feels misplaced.
I hope I covered it in my prev. reply to Jeroen Habraken.
... 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,
I am not sure if the underlying convert *design* is any superior compared to the lexical_cast design. Although, I'd note that convert provides considerably more room for specialization and optimization. However, as far as the functionality is concerned (something that users might be interested in), then I have a feeling that 'convert' provides quite a bit more. From the "Introduction" section: * throwing and non-throwing behavior when conversion fails; * support for the default/fallback value to be returned when conversion fails; * two types of the conversion-failure check - basic/simple and better/safe; * formatting support based on the standard std::streams and std::stream-based manipulators (like std::hex, std::scientific, etc.); * support for different locales; * support for boost::range-compliant char and wchar_t-based string containers (std::string, std::wstring, char const*, wchar_t const*, char array[], etc.); * no DefaultConstructibility requirement for the Target/Destination type; * extendibility and additional room to grow. But I am sure you've already read that section. So, I am a bit at a loss what to add to it.
... why it couldn't been implemented as an extension to that library.
I tried covering that in my prev. reply to Jeroen Habraken.
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.
What is std::u8string?
You've got me. Unfortunately, there is no such a thing. Wishful thinking on my part. I'd like to have one though. :-)
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.
Almost any feature is experimental.
Well, that's certainly wrong (IMHO of course).
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. Best, V.
On Wed, Apr 27, 2011 at 3:13 AM, Vladimir Batov <vbatov@people.net.au> 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.
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.
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".
participants (2)
-
Thomas Heller -
Vladimir Batov