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.