3

I have this simple function that, given a string str, if it is a number then return 'true' and overwrite the reference input num.

template <typename T>
bool toNumber(string str, T& num)
{
    bool bRet = false;
    if(str.length() > 0U)
    {
        if (str == "0")
        {
            num = static_cast<T>(0);
            bRet = true;
        }
        else
        {
            std::stringstream ss;
            ss << str;
            ss >> num;    // if str is not a number op>> it will assign 0 to num
            if (num == static_cast<T>(0)) 
            {
                bRet = false;
            }
            else
            {
                bRet = true;
            }
        }
    }
    else
    {
        bRet = false;
    }
    return bRet;
}

So I expect that:

int x, y;
toNumber("90", x); // return true and x is 90
toNumber("New York", y); // return false and let y unasigned.

On my machine, both debug and release configurations works fine, but on the server, only with the debug configuration, in calls like toNumber("New York", y) the 'ss >> num' fails to recognize that str is a string.

I checked the project configuration, but they are the same for both machines (the server is a svn-checkout of my local vs-2015 project).

I have literally no idea on how to solve the problem. Can anyone help me with this?

Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
GiaMat45
  • 55
  • 7

3 Answers3

5

Checking the value of the number that operator>> outputs is the wrong way to go. You should check the stringstream's failure state instead, eg:

template <typename T>
bool toNumber(string str, T& num)
{
    bool bRet = false;
    if (str.length() > 0U)
    {
        if (str == "0")
        {
            num = static_cast<T>(0);
            bRet = true;
        }
        else
        {
            std::stringstream ss;
            ss << str;
            if (ss >> num) // <--
            {
                bRet = true;
            }
            else
            {
                bRet = false;
            }
        }
    }
    else
    {
        bRet = false;
    }
    return bRet;
}

operator>> returns a reference to the input stream, and the stream is implicitly convertible to bool in boolean contexts, like an if statement, where true means the stream is in a good state, and false means the stream is in a failed state.

In which case, you can greatly simplify the function much much further, by getting rid of the redundant bRet assignments, and the redundant input checks that the stringstream will already handle for you, eg:

template <typename T>
bool toNumber(const string &str, T& num)
{
    std::istringstream ss(str);
    return static_cast<bool>(ss >> num);
    // or:
    // return !!(ss >> num);
}

Online Demo

Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • Thanks for the hint but `return (ss >> num)` does not return a bool, then the compiler throw this error `'return': cannot convert from 'std::basic_istream>' to 'bool' ` – GiaMat45 Nov 03 '22 at 16:25
  • Ok now with the cast works very fine! I will take this very-compact version. Thks so much! – GiaMat45 Nov 03 '22 at 16:42
1

Your code is made too complicated, you can simplify it to this:

template <typename T>
bool toNumber(std::string str, T& num)
{
    return !!(std::istringstream { std::move(str) } >> num);
}

https://godbolt.org/z/Pq5xGdof5

Ok I've missed that you wish to avoid zero assignment in case of failure (what streams do by default):

template <typename T>
bool toNumber(std::string str, T& num)
{
    T tmp;
    std::istringstream stream{ std::move(str) };
    if (stream >> tmp) {
        num = tmp;
    }
    return !!stream;
}

https://godbolt.org/z/nErqn3YYG

Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
Marek R
  • 32,568
  • 6
  • 55
  • 140
  • Nothing in the OP's question indicates that `num` should be left unassigned when `false` is returned. Doing so just introduces an unnecessary logic branch. Case in point, the OP's original code does zero-assign `num` if `str` is not empty or `"0"` and the stream fails. – Remy Lebeau Nov 03 '22 at 16:35
  • In code comments are indicating that: `// return false and let y unasigned.` and `// if str is not a number op>> it will assign 0 to num`. Not very directly, but still. And when you think why his code is so complex this must be a root cause (for example this: `if (str == "0")` must be for that). – Marek R Nov 03 '22 at 17:19
  • The way the OP's code is written, `return false and let y unasigned` is misleading, it is actually more like `return false and y may or may not be left unasigned` since `if str is not a number op>> it will assign 0 to num` is true. `y` would be left unassigned only if `str` is empty, but will be assigned `0` if `stringstream` fails. Not very consistent behavior. The comment should really be more like `return false and ignore y` – Remy Lebeau Nov 03 '22 at 18:12
0

if (num == static_cast<T>(0))

Bad idea. To know if operator>> failed, check the status of the stream.

if (ss >> num) { ...

Once you have this check in place, the (totally incorrect) piece

    if (str == "0")
    {
        num = static_cast<T>(0);
        bRet = true;
    }

becomes redundant, and

if(str.length() > 0U)

also becomes redundant, and then the whole thing simplifies to a line or two.

n. m. could be an AI
  • 112,515
  • 14
  • 128
  • 243