Custom string-to-int converter

Question

I have written the following function:

#include <iostream>
#include <utility>
#include <string>
#include <string_view>
#include <limits>
#include <optional>
#include <stdexcept>


// header file

std::optional<int> isInteger( const std::string_view token, const std::pair<int, int> acceptableRange =
                              std::pair<int, int>( std::numeric_limits<int>::min( ), std::numeric_limits<int>::max( ) ) ) noexcept;

// source file

std::optional<int> isInteger( const std::string_view token, const std::pair<int, int> acceptableRange ) noexcept
{
    if ( token.empty( ) )
    {
        return { }; // not convertible
    }

    try
    {
        std::size_t pos { 0 };

        const int result_integer { std::stoi( std::string{ token }, &pos, 10 ) };

        const auto& [ minAcceptableValue, maxAcceptableValue ] { acceptableRange };

        if ( pos == token.length( ) && ( result_integer <= maxAcceptableValue && result_integer >= minAcceptableValue ) )
        {
            return result_integer; // convertible, this is the happy ending
        }
        else
        {
            return { }; // not convertible
        }
    }
    catch ( const std::invalid_argument& ia ) { return { }; } // not convertible
    catch ( const std::out_of_range& oor ) { return { }; } // not convertible
    catch ( const std::length_error& le ) { return { }; } // not convertible
    catch ( const std::bad_alloc& ba ) { return { }; } // not convertible
}


// call site

int main( )
{
    const std::string str { "12" };
    const std::pair<int, int> acceptableRange { std::make_pair<int, int>( 0, 50 ) };

    const std::optional<int> tempInteger { isInteger( str, acceptableRange ) };

    if ( tempInteger ) { std::cout << "Converted value: " << tempInteger.value( ) << '\n'; }
    else { std::cout << "Conversion failed" << '\n'; }
}

The above function converts any string_view to an int if possible, otherwise returns an empty std::optional. I thought that returning an empty optional acts as an error code and informs the caller of the failure. So there isn't any need to let it throw and then handle the exceptions in the call site. I think that stack unwinding is unnecessary in this case.

I have a few questions:

1. Is this function good in terms of performance? Or should I use an alternative from another library? If so, then what can be the alternative?
2. Does it cover all the edge cases? I have tested it with many different (valid and invalid) inputs but it never failed.
3. Is it good that it's handling the possible exceptions inside its scope?
4. I declared it noexcept and this decreased the size of the executable (just a little). Do you recommend doing this for this function since no exception will escape its body?
5. Can anything about this function be further improved?

Also I use GCC v11.2 (just in case you need to know).

Answer 1

1. Wrapping a convenience wrapper in a convenience wrapper is generally just wasteful. Instead of calling std::stol(), go directly for std::strtol().

   • No need to convert to a std::string, which is potentially expensive and can fail.
     Instead, after trimming yourself, reject if too long, otherwise copy into a fixed-length buffer and add terminator.

   • No need to catch any exceptions, as the wrapper adding them is avoided.

2. As an aside, if you want to catch all exceptions and treat them equally, use the catch-all catch(...). Even if you don't forget any, writing them out and verifying is just tedious, and potentially bloats your executable. The compiler is unlikely to figure out you are really catching all possible exceptions, and thus the superfluous runtime type checks stay.

3. Marking all functions which cannot throw by design (in contrast to those which do so by happenstance) noexcept is a good idea. It spells the contract out for users and lets the compiler take advantage too.

4. CV-qualifiers on parameters in pure declarations (not definitions) are just noise, as they have no effect on the functions signature.

5. I'm really not sold on packing the maximum and minimum into a pair, instead of passing them individually.

If you use pre-C++17 with a back-ported replacement for std::string_view, this code might be of interest:

auto trim(std::string_view s) noexcept {
    auto space = [](unsigned char c){ return std::isspace(c); };
    while (!s.empty() && space(s.front()))
        s.remove_prefix(1);
    while (!s.empty() && space(s.back()))
        s.remove_suffix(1);
    return s;
}

std::optional<int> parseInteger(std::string_view s, int min, int max) noexcept {
    s = trim(s);
    constexpr size_t nBuffer = std::numeric_limits<int>::digits / 3 + 3;
    if (!s.size() || s.size() >= nBuffer)
        return {};
    char buffer[nBuffer];
    std::copy(s.begin(), s.end(), buffer);
    nBuffer[s.size()] = 0;
    char* end;
    long r = std::strtol(buffer, &end, 10);
    if (end - buffer != s.size() || r < min || r > max)
        return {};
    return r;
}

6. Since C++17, you can use std::from_chars() for the parsing, which while a bit more basic, is also more amenable to fitting into your own code however you want. It avoids copying and even results in shorter code:

std::optional<int> parseInteger(std::string_view s, int min, int max) noexcept {
    s = trim(s);
    if (!s.size())
        return {};
    if (s.size() > 1 && s[0] == '+' && s[1] != '-')
        s.remove_prefix(1);
    int r;
    auto result = std::from_chars(s.begin(), s.end(), r, 10);
    if (result.ec || result.ptr != s.end() || r < min || r > max)
        return {};
    return r;
}

Answer 2

The name isInteger is curious. It suggests that the function is a predicate (i.e. returns a bool). I would go with something like toInteger or asInteger.

---

Some of the includes need to be present in the header file:

#include <limits>
#include <optional>
#include <string_view>
#include <utility>

And others in the implementation:

#include <stdexcept>
#include <string>

<iostream> is only needed by the main() function, not by any of the code under review.

---

Since the function is declared noexcept, I think this is one of the few cases where I'd accept catch (...) - all exceptions should result in an empty optional being returned.

---

I'd reformat the very long condition, and perhaps change the if { return } else { return } to not use else:

    const auto& [ min, max ] { acceptableRange };
    if (pos != token.length()) {
        return {};
    }
    if (result_integer > max || result_integer < min) {
        return {};
    }
    return result_integer;

---

This looks very much like a function that ought to be a template, perhaps with a couple of if constexpr branches to deal with signed and unsigned integers, and perhaps also floating-point types (with a rename to toNumber()).

Answer 3

Thanks to @Toby Speight and @Deduplicator I came up with a very compelling solution.

Here is my solution:

// header file

std::optional<int> to_integer( std::string_view token, const std::pair<int, int> acceptableRange =
                               { std::numeric_limits<int>::min( ), std::numeric_limits<int>::max( ) } ) noexcept;

// source file

std::optional<int> to_integer( std::string_view token, const std::pair<int, int> acceptableRange ) noexcept
{
    if ( token.empty( ) )
    {
        return { };
    }

    if ( token.size( ) > 1 && token[ 0 ] == '+' && token[ 1 ] != '-' ) { token.remove_prefix( 1 ); }

    int value { };
    const auto [ ptr, ec ] { std::from_chars( token.begin( ), token.end( ), value, 10 ) };

    const auto& [ minAcceptableValue, maxAcceptableValue ] { acceptableRange };

    if ( ec != std::errc( ) || ptr != token.end( ) ||
         value < minAcceptableValue || value > maxAcceptableValue ) { return { }; }

    return value;
}

The execution speed improvement ranges between ~20% to ~1450% depending on the input string. Also, the above solution slightly reduced the size of the executable.

This solution does not involve work with std::string and exceptions, thus the result is performance gain and probably a smaller memory footprint.