Boost logo

Boost :

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.


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk