Subject: Re: [boost] [review] string convert
From: Vladimir Batov (vbatov_at_[hidden])
Date: 2011-05-03 08:36:32
> 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.
> 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"
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.