Hi Vladimir, all, First off, I'll respond to Rob and Jeremy. Here's again the alternate "simpler" syntax which I put in my original review, which I think must have gotten buried from typing too much. :-/ optional<int> i = string_convert<int, dont_throw>(s); // or string_convert<int>(s, _throw=false) if that's impossible int i = string_convert<int>(s, manip_ = std::hex); Well, Jeremy pointed out
You propose that a bunch of tag_ names be used, but in practice they would likely have to be something like boost::convert::tag_, which would substantially reduce the benefits of any such change.
So, yes, I guess dont_throw or any tag type put into the template parameters, is basically disqualified by its verbosity. That's too bad, because it would be nice to have a single function that returned either optional or a value. I suppose that Vladimir's nice named parameters exhibit the same problem. Yuck. Nonetheless, my objections and simplifications are quite orthogonal. On May 2, 2011, at 10:35 PM, Vladimir Batov wrote:
If the library is accepted (which seem unlikely so far and that's truly OK with me), then I am prepared to discuss and try out alternative approaches (if they are in some concrete "try-able" form).
Yes this is the review deadlock/stalemate problem I referred to earlier. Anyway, I remain in favor of the library with simplified semantics. It only boils down to three things I find distressing. 1. A special context which causes an apparent function call to either throw or not throw. 2. A streaming operation outside an apparent function call which applies IO manipulators within it. 3. "::from" I'll drop #4, the multiple operator(), because that's common practice and it's quite clear what that's doing.
The reason I've "come up" with the API as it is now is because during discussions some 2-3 years ago I in fact tried various proposed interfaces as we were discussing them. The results of those trial-n-errors is this current API.
I apologize that I still haven't reread those. From what I remember, these issues were raised but I don't remember whether there was consensus how to fix them. Despite that this review might return a negative result, I think it's been a valuable discussion.
It is incredibly clear what lexical_cast does because it uses the standard meaning of the operators, function calls, and return values.
int i = lexical_cast<int>(str); int i = convert<int>::from(str);
When I look at the above where lexical_cast and 'convert' provide the same functionality, I am not sure if from the user perspectives I immediately see how 'convert' differs from lexical_cast complexity-wise. I am certainly biased.
It's two more tokens, and I don't see the value to the user of separating the two parameters. I admit it's partly a matter of taste, but I think following the lexical_cast syntax as far as possible has objective value. Since you say it's there for technical reasons, let's take a look at how Vicente fixed it in his Conversion library. I think this is the relevant section of his doc: " How to specialize the conversion functions You can add an overload of convert_to and assign_to functions as you will do for the swap function for example, but we have to use a trick to allow ADL and avoid infinite recursion and ambiguity. This trick consists in adding a unused parameter representing the target type. E.g. if you want to add an explicit conversion from a type A to a type B do the following: namespace my_own_namespace { B convert_to(const A& from, boost::dummy::type_tag<B> const&); } "
I'd try distinguishing "not simple enough" and "annoying". The *user* API is one (!) function -- convert::from -- on one side and *optionally* convert::result on the receiving side. I really do not know how to simplify it any further. I personally do not consider changing that "annoying" from() to something else to be a simplification. So, by "simplification" you probably had something else in mind and I am all prepared to listen.
The complexity I and others are referring to is obviously not the number of functions! It's the fact that the converter<> object returned by from() has at least three different behaviors, when I think most people from looking at it would think conversion has already happened and a simple value is being returned. - It can throw if assigned to a regular value - It doesn't throw if assigned to ::result - It can be a functor which takes a string and returns a value, if the string was omitted in the constructor (?) or if the string was empty (?)
As for "I don't like "::from" at all", then it's a personal choice and we'll never get a consensus here.
True.
The practical reason for "::from" is two-fold -- 1) it clearly shows the direction of the conversion (which's been deemed important during original discussions);
Again, I haven't reread those discussions. I am surprised that people would find a function call confusing. Usually it takes its input as an argument and returns its result. :-p
2) the syntax has to be convert<TypeOut>::from<TypeIn> (replace 'convert' and 'from' with the identifiers of your choice), i.e. two types need to be evaluated separately. Otherwise, various specializations (if there are such) conflict. That's why the lexical_cast-like API (where both TypeIn and TypeOut evaluated at the same time) does not work (at least I did not manage it to work).
Again, see Vicente's library for the solution.
... seem to require this weird static member syntax.
I am not sure I am following. What's exactly weird about it (apart from your personal syntax preferences)? It's the standard C++ syntax, right?
I guess I'd expect something "more" to happen, like a side effect of instantiating the convert<> template, or having some value to instantiating a convert<> object or something. Instead it's really just a function call where some of the template parameters are pushed into a class. At least now I understand why you're doing this, although it appears to be unnecessary.
So, next '::result'. It's some form of optional return. So why not just return optional? It is not much less to type convert<int>::result maybe = convert<int>::from("not an int"); versus optional<int> maybe = convert_cast<int, use_opt>("not an int");
Yes, convert::result does not offer *much* beyond boost::optional. However, it *does* offer more. That's the reason it exists. Namely, convert::result provides two pieces of information -- if the conversion failed/succeeded *and* the fallback value (if provided). boost::optional provides one *or* the other. The difference might be considered subtle but it's there.
So the intention is something like this? convert<int>::result i = convert<int>::from(s, 17); if(!i) return i.value(); // returns 17 (!) That looks pretty weird to me. I don't see why someone would want to specify the default value if they are then going to check if conversion succeeded. If that's really wanted, I think simply pair<bool,T> would be more clear.
Why should the LHS modify the behavior of the proxy object returned by ::from?
Because usages of 'convert' differ -- namely, throwing and non-throwing behavior. I personally do not use throwing behavior at all but I feel it was needed to provide/support the behavior (and resulting mess) for lexical_cast backward compatibility and to cater for usages different from mine.
Personally I would want both behaviors. It's just the context-sensitive value that I object to. Is the following syntax possible? I am not up on Boost.Parameter yet. int i = convert<int>::from(s)(_dothrow); optional<int> j = convert<int>::from(s)(_dontthrow);
It's far too much to think about for such a simple operation.
Not really. Well, I do not see it that way anyway.
convert<int>::result res = convert<int>::from("blah", -1); if (!res) message("conversion failed"); int value = res.value(); // proceed with the fallback anyway.
The locality of 'convert' application (IMHO of course) does not leave much to think how 'convert' is to behave.
Clear enough in itself (although I'm still reeling from the idea of an object which converts to false having a value). It's just that the next line might read int res2 = convert<int>::from("blah"); and throw, and there's no indication why one version throws and the other does not.
The huge *practical* advantage of operator() is chaining. I personally do not see one or the other of the below simpler or harder and both are of equal length:
from(s, _manip = std::hex); from(s)(_manip = std::hex);
However, the first one has the limit on the number of args. Second does not. Second is much easier/cleaner to implement (therefore, to maintain which is even more important) as #1 requires from() overloading or def. parameters. Thirdly, second looks more natural as a progression of increasing complexity as the user gains additional knowledge:
int i = from(s); // simplest int i = from(s)(_manip = std::hex); int i = from(s)(_manip = std::hex)(locale_ = ...);
Yeah, I get this now. Thanks!
On a similar nice-feature-but-weird-syntax note, let's look at the Using Boost.Convert with Standard Algorithms section, which offers a few ways that you can use convert<T>::from() to create a functor:
std::transform( strings.begin(), strings.end(), std::back_inserter(integers), convert<int>::from(string(), -1) >> std::hex); ... I think it would be much more appropriate to provide a Phoenix function object for the purpose, something like std::transform( strings.begin(), strings.end(), std::back_inserter(integers), convert_<int>(_1, -1, std::hex)); Sigh of relief. I see what that means immediately, ...
Apologies for sounding as a broken record but again that preference seems quite personal. As a user not familiar with Phoenix your variant would not look familiar to me. In order to use it I'd need to learn/remember/know additional API -- convert_, _1, args order. My deployment seems simpler (not surprisingly some bias here :-) ) as the user uses the same API and does not need to learn more to use 'convert' with algorithms.
Mentioning that not for the sake of arguing tooth and nail for *my* API. I am happy to consider alternatives. I just do not see your suggested API as a clear winner (not yet anyway).
I think Boost.Lambda/Boost.Phoenix is pretty well-known syntax by now, but perhaps I've been indoctrinated. I guess your syntax is a bit like currying - drop an argument and it becomes a function object which takes that argument. Don't think I've seen that in C++ before.
I also want to mention type-to-type conversion, which Vicente Botet has thought out much more thoroughly in his Conversion library. This library offers type-to-type conversion seemingly as an afterthought, just because the syntax allows it.
I have to disagree. Again I consider 'convert' to be more of a conversion framework rather than a library. Indeed, 'convert' does not provide much more beyond string-to-type conversions because I personally did/do not need more. However, it does not mean I/we should deny that possibility to others as their needs may differ. Therefore, 'convert' was designed with that in mind. If Vicente needs type-to-type conversion functionality, I think he might consider implementing that functionality within the 'convert' framework by extending the 'convert' library for everyone's benefit.
Leaving aside which is a more general framework... I believe yours falls back to type-to-string-to-type whereas his does not. Both are valid behaviors.
I do not think just one person can provide every possible conversion. I cannot anyway. And with 'convert' I am not saying I do.
Extensibility is essential.
I believe I did implement the functionality that I immediately needed. More so, I tried to address concerns/suggestions brought up while the library was being written. So, I feel it might be a good basis from which we could move *forward* improving it -- adding conversions others needed, optimizing existing conversions.
I do think it's a good start.
Maybe an alternative might be to delay reviewing this library for a year (?) and let guys try extending lexical_cast as there were numerous suggestions. Then, we'll re-evaluate the situation. Although my suspicion is that in 1-2 years that lexical_cast-extension push will fizzle out once again, new people will come in asking the same questions and we won't move an inch.
I think I made suggestions which answer almost all of the concerns raised in this review, except for the insistence on lexical_cast. The functionality is clearly there. It's just this odd use of Expression Templates where IMO it's not needed, that irks me. For what? To specify a context where a function won't throw. To apply io manipulators using a familiar operator. As a shortcut for lambdas. Thanks for explaining all of this - I feel a lot better knowing why the library is how it is, especially that peculiar ::from. I still disagree and hope you'll consider the "simplifications" I've suggested. (I doubt they are original in any way!) If not, there'll probably be another lexical_cast debate soon enough. Cheers, Gordon