Re: [boost] [review] string convert

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.