Re: [boost] [review] string convert

Subject: Re: [boost] [review] string convert
From: Stewart, Robert (Robert.Stewart_at_[hidden])
Date: 2011-05-04 10:33:53

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);

> // 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.

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.  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.

_____
Rob Stewart                           robert.stewart_at_[hidden]
Software Engineer                     using std::disclaimer;
Dev Tools & Components
Susquehanna International Group, LLP  http://www.sig.com

IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.