Gordon Woodhull <gordon <at> woodhull.com> writes: .. 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);
Apologies if you expected me to reply to something that I missed. I believe the functionality you are proposing is available via convert<int>::result res = convert<int>::from(s); The deployment of boost::optional might be quite appropriate in this particular case. However, I fee that the deployment of dedicated convert::result is better suited across (uniformity) all library uses. I'd probably like to hear more what clear advantages your proposed interface offers over the existing one.
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. ... I suppose that Vladimir's nice named parameters exhibit the same problem. Yuck.
Yes, my examples have or assume using namespace boost; using namespace boost::conversion::parameter;
... Anyway, I remain in favor of the library with simplified semantics. It only boils down to three things I find distressing.
I am not sure it's not 'distressing'. More like something you are not immediately comfortable with, right? ;-) I certainly do not want anyone distressed over something I suggest.
manipulators within it.
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 3. "::from"
Given that I'll have to juggle different suggestions/comments,/etc., I can't promise I'll change the above as you request (depending on the review outcome). I promise thought to make an honest effort to address your and all other concerns raised during the review. If ultimately I will not be able to proceed as you (or anyone else) suggested, then I'll try provide my reasoning behind my decision. I understand it's far from "yes, I'll fix #1-3" but that's all I can responsibly say at this moment.
int i = lexical_cast<int>(str); int i = convert<int>::from(str);
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.
I thought it was one more token rather than two (lexica_cast=1; convert+from=2; 2-1=1). And as a user I probably do not care one way or the other. Given I am not convinced that boost::optional does it as well as convert::result I might be still leaning towards convert::from so that convert::result did not feel lonely. :-)
Since you say it's there for technical reasons, let's take a look at how Vicente fixed it in his Conversion library.
Yes, thans for the pointer. Looked briefly and it looked promising. Putting on my TODO list.
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 (?)
It's almost as good as a swiss knife! On a serious note though I was adding the functionality as the relevant need arose. What should I do instead?
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
From
convert<int>(std::string) it's not immediately clear if we convert int-to-str or str-to-int. So, more explicit direction was decided would be useful/helpful. Reading convert_to<int>(str) or convert<int>::from(str) is more natural to read and easier-flowing and kind of self-documenting (I feel that way anyway).
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.
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.
For better or worse my usual processing flow of configuration parameters: // Try reading a configuration parameter. convert<int>::result i = convert<int>::from(s, 17); // Log error if parameter was bad. if(!i) message ("parameter ignored. default used"); // Proceed with the default. return i.value(); // returns 17 (!)
If that's really wanted, I think simply pair<bool,T> would be more clear.
Well, so far you suggest boost::optional in one place std::pair in another. I feel that providing one convert::result is more polished and easier to grasp. I personally is botherd by std::pair<iterator, bool> from std::map::insert(). Equally I am not thrilled about pair<bool,T> in this context... more so because it'll probably be pair<bool, boost::optional<T> > -- something convert::result encapsulates.
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.
Agreed.
It's just the context-sensitive value that I object to.
Yes, it probably takes some getting used to but in reality I do not think it's that bad. People are likely to use one or the other. Like I personally always use the non-throwing version.
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);
Yes, it is possible. However, in #1 (_dothrow) is stating the obvious. We have to throw as there is nothing to return when failed. So, initially your #1 suggestion might be useful (if only for documenting purposes) but later I am afraid bothersome. #2 is available as convert<int>::result res = convert<int>::from(s); So, essentially we are talking about different deployment/API of the same functionality. I'll take your concern on board and we'll try addressing it in due course.
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.
Understood. I have no answer to that. Different cases, different behavior. If I could make them behave the same, I would. Any suggestions how to address that are most welcomed.
I think Boost.Lambda/Boost.Phoenix is pretty well-known syntax by now, but perhaps I've been indoctrinated.
Nop. Tried both but was happier with boost::bind and boost::regex. Maybe due to my use-cases, maybe due to something else. ;-)
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.
Could that be a good thing? :-)
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.
As I said, I can't promise as I'll have to take into account other suggestions. However, I'll make a genuine effort and try incorporating your suggestions... unless it is a matter of one API over another API. Then we can't win that battle no matter which way we go.
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.
I really do not see it as a big deal. To me the library's policy is -- it won't throw unless it is the only thing it can do. Like lexical_cast really. Still, we could consider adding something like
int i = convert<int>::from(s)(_dothrow); optional<int> j = convert<int>::from(s)(_dontthrow);
to make the behavior more explicit.
To apply io manipulators using a familiar operator.
I remember considering int i = convert<int>::from(s)(format_ = std::hex); or something along these lines. Can't see it as a problem adding. Removing op>>() though might be harder as I remember people fairly evenly split supporting and disliking the existing op>>()-based interface.
As a shortcut for lambdas.
The returned from convert::from() converter is a functor which we then feed to an algorithm. I fairly standard deployment, don't you think? Thanks for all your input. It's much and truly appreciated. V.