Hi, My review: -- What is your evaluation of the design? The overall design is very flexible, there are a few points though: - Although this might be a matter of taste I find the choice of operator >> to add modifiers rather odd, it smells a bit like operator abuse. Also, it forces you to use braces in std::cout << (boost::convert<int>::from("12") >> std::hex) << "\n"; for example, maybe using a member function is an option. - In might be nice to add a way to return a default-constructed object to the non-throwing interface. This is useful in combination with boost::optional: boost::convert<boost::optional<int>
::from_default("XXX") returning an boost::optional<int>() instead of having to write boost::convert<boost::optional<int> >::from("XXX", boost::optional<int>());.
-- What is your evaluation of the implementation? Had a quick read through the code, it looks fine except that names containing "__" are reserved (used in the include guards). -- What is your evaluation of the documentation? The documentation covers all aspects of the library quite nicely. This in contrast to the reference which seems mostly empty, in my opinion it doesn't add much and can simply be removed. Since the behaviour of convert seems to be different to lexical_cast (see below) it might be useful to add a section listing these differences. Lastly the documentation contains a couple of examples, it would be nice to have these as separate files in libs/convert/example. -- What is your evaluation of the potential usefulness of the library? The library is very flexible and the extra features are nice, however there is a rather large problem, in my opinion. This is the domain of boost::lexical_cast, a library that has matured over the years and is in heavy use. It can not be deprecated or worse, removed. Even though it has problems (mostly missing functionality, which boost::convert has implemented) I do not believe adding a second library is a proper solution. Instead this functionality should be added to boost::lexical_cast itself (and patches to do so have been proposed over the years). -- Did you try to use the library? With what compiler? Did you have any problems? Yes, on Mac OS X with both gcc 4.2.1 (the default shipped by Apple) and 4.5.2. Problems: - boost::convert<int>::from("-123.2") behaves differently from boost::lexical_cast<int>("-123.2"), the former returns -123 whilst the latter throws. Whilst I'm not saying one is better than the other, the differences should be documented for people switching. - boost::convert<int>::from('1') results in a rather long error message (http://codepad.org/YgoR8H2t), if the conversion from X to Y isn't possibly this should be made clear in a more user-friendly way. -- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I've read the documentation, had a quick glance at the source code and tried the basic functionality of the library. -- Are you knowledgeable about the problem domain? As the author of boost::coerce I'm familiar with the conversion domain, however I'm less familiar with the locale domain. -- Do you think the library should be accepted as a Boost library? No it should not, instead the extra functionality should be added to boost::lexical_cast. Kind regards, Jeroen Habraken
Jeroen Habraken <vexocide <at> gmail.com> writes: Hi, My review:
Jeroen, Thank you for your input. It's much appreciated. Please let me address/reply to a few points that you make if not in an attempt to change your mind ;-) but for the benefits of other readers (if there are such).
Although this might be a matter of taste I find the choice of operator >> to add modifiers rather odd, it smells a bit like operator abuse. Also, it forces you to use braces in std::cout << (boost::convert<int>::from("12") >> std::hex) << "\n"; for example, maybe using a member function is an option.
Yes, indeed operator>> is the matter of taste and it's not exactly my taste either. In fact, at one stage I had the (manipulator_ = std::hex) interface available to align with (locale_ = ...), etc. As nothing is carved in stone we might discuss bringing that interface back (if it is decided to proceed with the library of course). Still, after discussions we settled on operator>> as we felt it was complimentary to the standard std::stream-based manipulators and, therefore, immediately familiar to the potential user.
In might be nice to add a way to return a default-constructed object to the non-throwing interface. This is useful in combination with boost::optional: boost::convert<boost::optional<int> ::from_default("XXX") returning an boost::optional<int>() instead of having to write boost::convert<boost::optional<int> >::from("XXX", boost::optional<int>());.
First, I have to disagree on returning a default-constructed object. Here we are likely to differ in our interpretations/treatments of the default constructor. To me it's just another constructor which happens not to have any arguments. You seem to treat that constructor as some kind of special/default even though in general terms it might not be even available for a class. Secondly, I admit I've never thought of the above-mentioned deployment like boost::convert<boost::optional<int>. In fact, I am still having difficulties coming up with reasonable use-cases for such a deployment. I thought convert<int>::result does what you are seemingly trying to achieve with boost::optional.
The documentation covers all aspects of the library quite nicely. This in contrast to the reference which seems mostly empty, in my opinion it doesn't add much and can simply be removed.
Yes, I readily agree that the Reference section is not perfect. ;-) I've been struggling with Doxygen deployment as part of QuickBook processing. That is on my TODO list (if it is decided to proceed with the library of course).
Since the behaviour of convert seems to be different to lexical_cast (see below) it might be useful to add a section listing these differences. Lastly the documentation contains a couple of examples, it would be nice to have these as separate files in libs/convert/example.
Both of your suggestions make sense to me and I'll look into addressing them (the disclaimer again - if it is decided to proceed with the library).
boost::convert<int>::from("-123.2") behaves differently from boost::lexical_cast<int>("-123.2"), the former returns -123 whilst the latter throws. Whilst I'm not saying one is better than the other, the differences should be documented for people switching.
I feel that lexical_cast behavior is correct. I'll look into addressing the issue (the disclaimer again - if it is decided to proceed with the library).
boost::convert<int>::from('1') results in a rather long error message ... this should be made clear in a more user-friendly way.
Yes, haven't I noticed? ;-) That problem is not exactly the convert lib problem and I believe is being addressed by C++0x. Still, I'll look into addressing the issue (the disclaimer again - if it is decided to proceed with the library).
Do you think the library should be accepted as a Boost library? No it should not, instead the extra functionality should be added to boost::lexical_cast.
I see that you are quite firm in your opinion that "the extra functionality should be added to boost::lexical_cast". ;-) And you know what? That was my original request a few years back. Unfortunately, the cruel reality of life does not seem to give a hoot about what we think would be the best way to proceed. So, one can just sit there adamantly insisting on nothing less but his never-happening "ideal solution" or one can accept the reality and adapt to the circumstances. I needed a real solution to my problems. I had to implement them. As I understand "starting fresh" was a collective decision on this list quite some time back. That is the reason this convert thing is currently under the review. I again urge you to please dig the archives to see why that "extra functionality should be added to boost::lexical_cast" view is not exactly original and most importantly why it was decided *not* to proceed as you insist. More so, after having to implement all that functionality let me assure you that I've come to a firm conclusion that that additional functionality/configurability is *not* implementable within the boundaries of the lexical_cast interface. In particular, the TypeIn and TypeOut types must be discriminated, i.e. one type has to be specialized first. Without going into details I'll just say that otherwise is just does not work. So, it has to be convert<TypeOut>::from<TypeIn>. You can take my word for it or you could walk that road yourself. Regardless of your decision (to believe me or try it out yourself) I feel it is quite short-sighted to deny the users functionality they need. I perfectly understand that this library is nothing revolutionary (like shared_ptr, bind, Spirit) but people seem to have need for that. Simply go to the Vault and see how many downloads of that convert has been so far compared to other libs. As for lexical_cast "can not be deprecated or worse, removed", then I used to use lexical_cast a *lot*. I do not use it anymore and my world did not implode. The reason -- it does not do what I need it to do. Therefore, the impact of deprecating lexical_cast might not be as disastrous as you might imagine. Things are being deprecated every day -- vinyl records, magnetic tapes, CDs, (long list follows) -- it's called progress. ;-) Best, V.
On 27 April 2011 00:47, Vladimir Batov <vbatov@people.net.au> wrote:
Jeroen Habraken <vexocide <at> gmail.com> writes: Hi, My review:
Jeroen,
Thank you for your input. It's much appreciated. Please let me address/reply to a few points that you make if not in an attempt to change your mind ;-) but for the benefits of other readers (if there are such).
Although this might be a matter of taste I find the choice of operator >> to add modifiers rather odd, it smells a bit like operator abuse. Also, it forces you to use braces in std::cout << (boost::convert<int>::from("12") >> std::hex) << "\n"; for example, maybe using a member function is an option.
Yes, indeed operator>> is the matter of taste and it's not exactly my taste either. In fact, at one stage I had the (manipulator_ = std::hex) interface available to align with (locale_ = ...), etc. As nothing is carved in stone we might discuss bringing that interface back (if it is decided to proceed with the library of course). Still, after discussions we settled on operator>> as we felt it was complimentary to the standard std::stream-based manipulators and, therefore, immediately familiar to the potential user.
Do you have a link to this discussion (if it was on a mailing list), I'm curious as to how this decision was reached?
In might be nice to add a way to return a default-constructed object to the non-throwing interface. This is useful in combination with boost::optional: boost::convert<boost::optional<int> ::from_default("XXX") returning an boost::optional<int>() instead of having to write boost::convert<boost::optional<int> >::from("XXX", boost::optional<int>());.
First, I have to disagree on returning a default-constructed object. Here we are likely to differ in our interpretations/treatments of the default constructor. To me it's just another constructor which happens not to have any arguments. You seem to treat that constructor as some kind of special/default even though in general terms it might not be even available for a class.
I do indeed consider it to be a special case but I can see your point.
Secondly, I admit I've never thought of the above-mentioned deployment like boost::convert<boost::optional<int>. In fact, I am still having difficulties coming up with reasonable use-cases for such a deployment. I thought convert<int>::result does what you are seemingly trying to achieve with boost::optional.
They seem to provide the same functionality but using boost::convert<int>::result when you need a boost::optional<int> in the end doesn't make sense. Can boost::convert<>::result be replaced by boost::optional?
The documentation covers all aspects of the library quite nicely. This in contrast to the reference which seems mostly empty, in my opinion it doesn't add much and can simply be removed.
Yes, I readily agree that the Reference section is not perfect. ;-) I've been struggling with Doxygen deployment as part of QuickBook processing. That is on my TODO list (if it is decided to proceed with the library of course).
Since the behaviour of convert seems to be different to lexical_cast (see below) it might be useful to add a section listing these differences. Lastly the documentation contains a couple of examples, it would be nice to have these as separate files in libs/convert/example.
Both of your suggestions make sense to me and I'll look into addressing them (the disclaimer again - if it is decided to proceed with the library).
Great.
boost::convert<int>::from("-123.2") behaves differently from boost::lexical_cast<int>("-123.2"), the former returns -123 whilst the latter throws. Whilst I'm not saying one is better than the other, the differences should be documented for people switching.
I feel that lexical_cast behavior is correct. I'll look into addressing the issue (the disclaimer again - if it is decided to proceed with the library).
boost::convert<int>::from('1') results in a rather long error message ... this should be made clear in a more user-friendly way.
Yes, haven't I noticed? ;-) That problem is not exactly the convert lib problem and I believe is being addressed by C++0x. Still, I'll look into addressing the issue (the disclaimer again - if it is decided to proceed with the library).
Eric Niebler argued that bad template errors are a library bug (see http://cpp-next.com/archive/2010/09/expressive-c-why-template-errors-suck-an...) and I agree with him, this should be fixed.
Do you think the library should be accepted as a Boost library? No it should not, instead the extra functionality should be added to boost::lexical_cast.
I see that you are quite firm in your opinion that "the extra functionality should be added to boost::lexical_cast". ;-) And you know what? That was my original request a few years back. Unfortunately, the cruel reality of life does not seem to give a hoot about what we think would be the best way to proceed. So, one can just sit there adamantly insisting on nothing less but his never-happening "ideal solution" or one can accept the reality and adapt to the circumstances. I needed a real solution to my problems. I had to implement them. As I understand "starting fresh" was a collective decision on this list quite some time back. That is the reason this convert thing is currently under the review. I again urge you to please dig the archives to see why that "extra functionality should be added to boost::lexical_cast" view is not exactly original and most importantly why it was decided *not* to proceed as you insist.
As you say this is a solution to your problem, not the problem that boost::lexical_cast has which should ultimately be addressed by someone. I'll have a look in the archives (do you happen to have a link?).
More so, after having to implement all that functionality let me assure you that I've come to a firm conclusion that that additional functionality/configurability is *not* implementable within the boundaries of the lexical_cast interface. In particular, the TypeIn and TypeOut types must be discriminated, i.e. one type has to be specialized first. Without going into details I'll just say that otherwise is just does not work. So, it has to be convert<TypeOut>::from<TypeIn>. You can take my word for it or you could walk that road yourself. Regardless of your decision (to believe me or try it out yourself) I feel it is quite short-sighted to deny the users functionality they need. I perfectly understand that this library is nothing revolutionary (like shared_ptr, bind, Spirit) but people seem to have need for that. Simply go to the Vault and see how many downloads of that convert has been so far compared to other libs.
I'm not saying your library doesn't solve a problem for users, it definitely does. However, it isn't the optimal solution in my opinion. Other approaches have been mentioned (see this thread for example, http://lists.boost.org/Archives/boost/2005/04/84296.php) but were rejected by the original author because he felt it should look like a cast. Since he has abandoned the library I believe it is time for someone to step up and take it to the next level. Anyways, we've had this discussion before.
As for lexical_cast "can not be deprecated or worse, removed", then I used to use lexical_cast a *lot*. I do not use it anymore and my world did not implode. The reason -- it does not do what I need it to do. Therefore, the impact of deprecating lexical_cast might not be as disastrous as you might imagine. Things are being deprecated every day -- vinyl records, magnetic tapes, CDs, (long list follows) -- it's called progress. ;-)
This analogy doesn't hold, a vinyl or CD can't be adapted to meet new or extra demands, boost::lexical_cast on the other hand can.
Best, V.
Regards, Jeroen
Jeroen Habraken <vexocide <at> gmail.com> writes: ... Do you have a link to this discussion (if it was on a mailing list), I'm curious as to how this decision was reached?
The discussions that I participated in were over two years back. I am pretty sure similar discussions/suggestions were well before that. So, no, I do not have those links but you can search for convert/review/lexical_cast/my name.
... I thought convert<int>::result does what you are seemingly trying to achieve with boost::optional.
They seem to provide the same functionality but using boost::convert<int>::result when you need a boost::optional<int> in the end doesn't make sense. Can boost::convert<>::result be replaced by boost::optional?
They, indeed, seem to provide *similar* functionality. In fact, convert::result *uses* boost::optional. If you have a look at convert::result (it's quite small), then everything else in addition to boost::optional is what convert::result adds. In words, IMHO convert::result is better than boost::optional tailored for its domain (conversions).
... Eric Niebler argued that bad template errors are a library bug (see http://cpp-next.com/archive/2010/09/expressive-c-why-template-errors-suck- and-what-you-can-do-about-it/) and I agree with him, this should be fixed.
Regardless of the outcome with the proposed library I'll read the article as indeed it seems quite interesting (I actually thought there was nothing we could do about those cryptic and lo-o-o-ong template-related error messages). Thanks.
As you say this is a solution to your problem, not the problem that boost::lexical_cast has which should ultimately be addressed by someone. I'll have a look in the archives (do you happen to have a link?).
Let me clarify. It's not *my* problem -- I have no problems. That's a problem of a project or projects that need to make extensive use of the functionality that 'convert' provides and lexical_cast does not.
I'm not saying your library doesn't solve a problem for users, it definitely does. However, it isn't the optimal solution in my opinion.
"Optimal" is a subjective thing. For one speed comes first, for some others richness of the interface is more important. For the projects that I am involved in the existing std::stream-based performance is a non-issue and locales and formatting are crucial. In other words, if something does not immediately match your interpretation of "optimal", it's not a good reason to discard it yet (obviously only if one stops looking at things from his-only molehill and starts taking other-peoples needs into consideration). What I am trying to say is that with 'convert' I am trying to give some users *something* and to keep working to satisfy the needs of the remaining users who need more. You, on the other hand, seem to prefer rejecting 'convert' outright and giving everyone nothing. Do you really think it is constructive or beneficial for anyone?
Other approaches have been mentioned (see this thread for example, http://lists.boost.org/Archives/boost/2005/04/84296.php)
You are teasing me, right? :-) Let me quote the link you provided -- "... a feature request, though: the ability to use a default value rather than throwing an exception on conversion failure. Basically, it would take a second argument that would be the value returned if the conversion fails." Isn't it the functionality that 'convert' *at very long last* provides? The example below is from the "Getting Started" section #include <boost/convert.hpp> using boost::convert; int i1 = convert<int>::from(str); // Throws if the conversion fails int i2 = convert<int>::from(str, -1); // Returns -1 if the conversion fails
but were rejected by the original author because he felt it should look like a cast.
Exactly. How many times do you need to be told that lexical_cast is closed for business and you should go and look someplace else? Instead, you behave like a spoiled child who simply refuses to accept things as they are and keeps nagging parents for something he won't get.
Since he has abandoned the library I believe it is time for someone to step up and take it to the next level.
How well are you qualified to make statements of that sort? Have you actually talked to the author? Or is it merely your impression, opinion, etc? My take of the situation is different (after I talked to the author and the then maintainer, endured many days of discussions on the list, went through the pain of actually reimplementing what was seemingly within the reach in the form of lexical_cast). So, my take of the situation is that the author is of an opinion that lexical_cast is *complete* as far as the original design envisaged by the author is concerned and, therefore, lexical_cast is closed for extension business. The fact that you might disagree with that opinion does not immediately give you rights to "take over" and to "take it to the next level". Your "next level" might not be someone else's "next level" and seemingly not the author's "next level". Deal with it.
As for lexical_cast "can not be deprecated or worse, removed", then I used to use lexical_cast a *lot*. I do not use it anymore and my world did not implode. The reason -- it does not do what I need it to do. Therefore, the impact of deprecating lexical_cast might not be as disastrous as you might imagine. Things are being deprecated every day -- vinyl records, magnetic tapes, CDs, (long list follows) -- it's called progress.
This analogy doesn't hold, a vinyl or CD can't be adapted to meet new or extra demands, boost::lexical_cast on the other hand can.
Of course the analogy holds. You indicated lexical_cast could "not be deprecated or worse, removed". I argued that *everything* is "depreciable". Could anyone imagine in my happy days that a computer mammoth like DEC will be deprecated, decimated, annihilated by a "printer" company? :-) Have a great day, V.
On 04/27/2011 04:07 PM, Vladimir Batov wrote:
Jeroen Habraken<vexocide<at> gmail.com> writes: ...
What I am trying to say is that with 'convert' I am trying to give some users *something* and to keep working to satisfy the needs of the remaining users who need more. You, on the other hand, seem to prefer rejecting 'convert' outright and giving everyone nothing. Do you really think it is constructive or beneficial for anyone?
I don't have an opinion on this library yet but hope to find time for a review of it. In the meantime, I thought I would comment on this statement. Variations came up in both the bigint and locale reviews. As I'm sure you know, Boost isn't just a collection of libraries. It strives to be a collection of "expertly designed" libraries. I don't remember "it is useful for some users" as being on the list of reasons to accept a library into Boost. Plenty of perfectly usable libraries have been rejected over the years simply because it could be better. I don't recall "giving everyone nothing" as an argument in the past but it sure has become popular recently.
Since he has abandoned the library I believe it is time for someone to step up and take it to the next level.
How well are you qualified to make statements of that sort? Have you actually talked to the author? Or is it merely your impression, opinion, etc? My take of the situation is different (after I talked to the author and the then maintainer, endured many days of discussions on the list, went through the pain of actually reimplementing what was seemingly within the reach in the form of lexical_cast).
In this same thread that you responded to a few weeks back the current maintainer stated he no longer has time and is happy if someone else can pick it up: <http://boost.2283326.n4.nabble.com/lexical-cast-version-of-lexical-cast-in-1-46-1-is-quite-older-than-in-trunk-td3413661.html> Clearly, that can mean different things to different people; however, if the library has no maintainer and the community decides that the best approach would be expanding lexical_cast to support these added features, then it will happen. Best regards - michael -- In the Sacramento/Folsom area? ** Profesional C++ Training mid-May ** http://www.objectmodelingdesigns.com/boostcon_deal.html Michael Caisse Object Modeling Designs www.objectmodelingdesigns.com
Michael Caisse <boost <at> objectmodelingdesigns.com> writes:
I don't have an opinion on this library yet but hope to find time for a review of it.
Please do. As IMHO if the situation does not improve, the library should not be considered for inclusion regardless how this current discussion progresses given how little interest the library generates (which is starkly different from the time the library was written/submitted).
In the meantime, I thought I would comment on this statement. Variations came up in both the bigint and locale reviews. As I'm sure you know, Boost isn't just a collection of libraries. It strives to be a collection of "expertly designed" libraries. I don't remember "it is useful for some users" as being on the list of reasons to accept a library into Boost. Plenty of perfectly usable libraries have been rejected over the years simply because it could be better. I don't recall "giving everyone nothing" as an argument in the past but it sure has become popular recently.
I certainly do see your point and somewhat agree with that. On the other hand, I favor pragmatism, actual results and gradual improvements over idealism and idle pondering. That "expertly designed" approach can be viewed in the right constructive context or instead viewed as elitist and can be too easily abused. The outcome -- workable, usable, immediately useful but not immediately perfect (which is relative anyway) libs are rejected at the whim of someone who may not even need/use that library in the first place. Who wins here? Not the users anyway. I do indeed appreciate the quality of Boost libs. However, I've never used Boost libs out of admiration of their "expert design". I used them because they were/are useful to me. Consequently, I will take something over nothing any day. Even more so with regard to convert/lexical_cast as those talks of "improving"/extending lexical_cast on this list date back at least to 2005. Participants and maintainers come and go. Talks, lexical_cast and the original lexical_cast design stay. And in IMHO the author is perfectly entitled to his view and to his vision. The author's put his name on a certain design and kindly offered it to others. Some people might disagree with the design and might like to do it differently. Fine. Go and do it outside lexical_cast. And that's what has been collectively decided before 'convert' started. I do not believe it is appropriate to approach it with the attitude -- "Yes, you, balding old man, thank you for all your efforts but now we'll take over and take it to the next level". As for "it is useful for some users", then I do not see anything wrong with that. In fact, isn't it *always* the case? No library will be useful for every user.
In this same thread that you responded to a few weeks back the current maintainer stated he no longer has time and is happy if someone else can pick it up: ...
Yes, I am aware of that. That situation has not dramatically changed from two years back when it was decided to proceed with 'convert' -- the author was not actively maintaining the library and then maintainer Alexander Nasonov was improving *underlying* implementation. Despite many suggestions extending interface had never been on the cards. What's changed? Unless now people are bold enough to say -- we'll take what you've done, we'll keep your name and from now on we'll do as *we* see fit. Not nice.
... if the library has no maintainer and the community decides that the best approach would be expanding lexical_cast to support these added features, then it will happen.
I wonder how you identify the "community". Is it those few who happen to be currently on the list vocal on the topic? Surely the original author is part of that community and on many occasions he expressed his view. What do we do with "inconvenient" views? Again, that topic of extending lexical_cast had place and resulted in submitting 'convert'. That was not even my idea. I was hoping Alexander Nasonov would do it all for me ;-) withing lexical_cast. Re-hashing all again that *after* the fact (the implementation based on our decision back then and subsequent submission) is exhausting. And, again, the provided by convert functionality/flexibility are not achievable within lexical_cast single-function interface. Are there any participants of those old convert/lexical_cast discussions still on the list to chime in? Best, V.
participants (3)
-
Jeroen Habraken -
Michael Caisse -
Vladimir Batov