Re: [boost] [review] string convert

Subject: Re: [boost] [review] string convert
From: Barend Gehrels (barend_at_[hidden])
Date: 2011-05-04 11:01:03

Hi Robert,

On 4-5-2011 16:33, Stewart, Robert wrote:
> Barend Gehrels wrote:
>
> [reformatted for readability, especially due to line wrapping]
>
>> int i = convert_cast<int>(s);
>>
>> // optional indicated in template parameter as return
>> // type - as always
>> optional<int>  i = convert_cast<optional<int>  >(s);
>>
>> int i = convert_cast<int>(s, 17);
>>
>> // template parameter as optional one there
>> int i = convert_cast
>>     <
>>        int
>>        , throw_even_though_i_specified_a_failback_
>>     >(s, 17);
>>
>> optional<int>  i ... (similar as other one with optional)
> I presume you mean the following, but why?
>
> optional<int>  i = convert_cast
>     <
>        optional<int>
>        , throw_even_though_i_specified_a_failback_
>     >(s, 17);
>
Good question - I included it because it was in the original list of 
Gordon, but I actually also wonder what is the specific usage of this 
function.

Gordon?

>> // pair indicated in template parameter
>> pair<bool, int>  i = convert_cast<pair<bool, int>  >(s, 17);
> convert_cast<int>(s) and convert_cast<int>(s, 17) are fine.
>
> convert_cast<optional<int>>(s) is too verbose.  As has been noted, a "try" variant would be much nicer: try_convert_cast<int>(s) would return an optional<int>.  Not only does that play nicely in C++03, but it's even nicer with auto.  I realize this loses the nice symmetry you were trying to achieve by using a single name for everything.
OK for me. I don't understand the advantage for auto here (as they 
return the same thing). But a TryParse equivalent sounds good to me. It 
tries and returns a null (optional) if failing.

> convert_cast<int, throw_>(s, 17) works pretty well (maybe "throw_on_failure").
>
> The only other one I haven't commented on is the pair<bool,int>  version.  It shouldn't require a second argument.

You mean the pair<bool,...> is redudant because there is also the 
version with optional. I agree.

>   All of the variations that take a single argument should have a version that takes a second in case there's no default construction or zero-initialization possible.  That aside, the pair version leads to much less readable code, not least because the semantics of pair<int,bool>  are less obvious than those of optional<int>.  Compare this:
>
> auto r(convert_cast<optional<int>>(s));
> if (r)
> {
>     i = r.get();
> }
>
> versus this:
>
> auto r(convert_cast<pair<int,bool>>(s));
> if (r.second)
> {
>     i = r.first;
> }
>
> Even better:
>
> auto r(try_convert_cast<int>(s));
> if (r)
> {
>     i = r.get();
> }
>
> Notice also that the precedent, std::map::insert()'s return type, puts bool second, not first.  I never remember the order and you used the reverse.  That leaves too much room for confusion.  The optional version is much better.

I agree completely. And indeed this usage with auto is convenient. One 
thing, users using it like this will not notice anymore that "auto" is 
(behind the screens now) an "optional", and might be surprised by the 
.get() call.

Regards, Barend