Re: [boost] [review] string convert

Subject: Re: [boost] [review] string convert
From: Vicente BOTET (vicente.botet_at_[hidden])
Date: 2011-05-04 15:39:12

> Message du 04/05/11 20:19
> De : "Stewart, Robert" 
> A : "'boost_at_[hidden]'" 
> Copie à : 
> Objet : Re: [boost] [review] string convert
> 
> Matthew Chambers wrote:
> > On 5/4/2011 11:05 AM, Vicente BOTET wrote:
> >
> > > I think we need to clarify one thing. Vladimir library uses
> > > values for two purposes:
> > > * as a default value when the type is not default
> > > constructible
> > > * as a fail-back in case of the conversion fails
> > >
> > > And I think we should mix them. To cover the first case we
> > > can use as I said in another post a default_value
> > > metafunction that can be specialized for the non default
> > > constructible type.
> 
> Presumably you meant we should *not* mix the two uses.

Yes.

> > > I agree that when a fail-back is given the user is not
> > > interested in knowing if the conversion succeeded or not, so
> > > in this case the return value should be T and not optional T.
> > > The question now is what function should be used,
> > > convert_cast or try_convert_cast. As the function doesn't
> > > throw I will use try_convert_cast, but as the function
> > > returns type T I will use convert_cast.
> >
> > I disagree. I think the fallback with conversion success is a
> > reasonable use case. Vladimir's case of notifying the user
> > when a fallback value is being used is reasonable. It's
> > difficult to leave that logic outside the conversion because
> > only the conversion knows if and why the input is invalid.
> 
> I agree with Matt.

I don't see the added value then. If the user needs to check if the conversion succeeded, why not setting herself the fail-back

int i;
if (! try_convert_to(s,i))
{
i = fail_back;
}

> > > Let me comment a little more on the function
> > > try_convert_cast returning optional T. The function can not
> > > be used directly where the target type T was expected so we
> > > can not consider it to follow the cast pattern.
> 
> You're right.
> 
> > > Other try_ functions return just bool. If we follow this
> > > pattern the preceding code could be written as
> > >
> > > int i;
> > > if (try_convert(s,i))
> > > {
> > > // do whatever you want with i;
> > > }
> 
> Yeah, I think that's right, too: a "try_" function should return bool. However, the name should be "try_convert_to" if you go that route.

OK.

> > > If you want to preserve the convert_cast that returns a
> > > optional T, I will prefer to name it optional_convert_cast,
> > > so the user that will read it will be advertised that the
> > > result is an optional T.
> > >
> > > auto r(optional_convert_cast(s));
> > > if (r)
> > > {
> > > i = r.get();
> > > }
> 
> I'd much rather see convert_cast>(S) than optional_convert_cast(). It indicates what is happening better.

Yes, this can be equivalent and doable.

> > If others agree, I could also go with Vincente's version of
> > try_convert. It doesn't bother me that passing in the variable
> > by reference makes it a two-liner, since the expression itself
> > can go inside the if statement and it implicitly supports
> > non-default-constructable types.
> 
> It is certainly non-surprising.
> 
> Using Vicente's default_value customization point still makes convert_cast> a viable candidate:
> 
> int i;
> if (try_convert_to(s)) 
> {
> // use i
> }

You missed the i parameter the call to try_convert_to.

> auto const c(convert_cast>(s));
> if (c)
> {
> // use c.get()
> }
> 
> Note that try_convert_to() makes for simpler, shorter, and more direct code and that both are two-liners.
> 
> > And we can also provide optional_convert, which CAN be
> > one-lined (if the user doesn't care about conversion success,
> > and if they DO, then they should use the try_ version!).
> 
> Offering both is possible, and depends upon the other functions in the set, though generally, offering two ways to do something can be a source of confusion.
>
> > string s = "4-2";
> >
> > // Matt likes to throw:
> > int i = convert_cast(s);
> >
> > // Vladimir cares about success:
> > int i = 17; if (!try_convert(s,i))
> > { /* log that fallback value is being used */ }
> >
> > // except when he doesn't (but he never throws)
> > int i = convert_cast(s, 17);
> >
> > // Vincente thinks success is optional:
> > optional i = optional_convert(s);
> >
> > Note that in several of these, target typename is no longer
> > needed, right?
> 
> try_convert(s, i) doesn't tell me whether s is being converted to i's type or i to s's type.

Is try_convert_to(s,i) better, or you see the same problem. Do you think it is better to use named parameters _from_ and _to?

> convert_cast(s, 17) has the same problem and breaks with the new-style cast pattern.

would you prefer to use a _failback named parameter? 

convert_cast(s, _failback=17)

or rename the function

if_fail_conversion_cast(s,17);

I don't really like the incidence on the interface/design of the failback feature. As no option satisfy completely I will just do

int i;
if (! try_convert_to(s,i)) i = 17;

If try_convert_to doesn't updates the target variable if conversion fails we can even write

int i=17;
try_convert_to(s,i);

> > With this setup, is there any reason that convert_cast and
> > optional_convert couldn't just be thin wrappers around
> > try_convert?

Yes, this seems a good implementation option.

> Perhaps, but there's still too much churn to think about that yet.

Yes, we need first to try to fix the interface.

Best,
Vicente