6
\$\begingroup\$

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

\$\endgroup\$

3 Answers 3

6
\$\begingroup\$
  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;
}
  1. 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;
}
\$\endgroup\$
1
4
\$\begingroup\$

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

\$\endgroup\$
4
  • \$\begingroup\$ Yes, I am thinking about making it a function template. However, I'm not that experienced with templates. I am not sure how I can handle unsigned too. \$\endgroup\$
    – digito_evo
    Jan 28 at 11:26
  • 1
    \$\begingroup\$ Unsigned types would be similar, but you'd use std::stoul() family instead. Or std::from_chars(), which will work better in a template, because it's already generic. \$\endgroup\$ Jan 28 at 13:27
  • \$\begingroup\$ @TobySpeight std::from_chars() is not actually generic. But there are overloads for all the integer and floating types, which has about the same effect. \$\endgroup\$ Jan 28 at 21:13
  • \$\begingroup\$ @Toby Speight I posted my final solution as an answer. Take a look at it if you would like to. \$\endgroup\$
    – digito_evo
    Jan 29 at 15:24
1
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ 1. const-qualifying the pair in the header just confuses the matter. 2. Initializing value is at best an insignificant pessimization. 3. Your modified code no longer ignores leading nor trailing whitespace. \$\endgroup\$ Jan 29 at 16:09
  • \$\begingroup\$ @Deduplicator Yes, I didn't include the trim function since I have a tokenize function which takes a string_view of user input and returns a vector containing tokens of type std::string (which do not have white space anymore). Then another function passes those tokens one by one to to_integer. \$\endgroup\$
    – digito_evo
    Jan 29 at 16:26
  • \$\begingroup\$ @Deduplicator So I should not initialize the value to get better performance? Is it safe to keep it uninitialized? \$\endgroup\$
    – digito_evo
    Jan 29 at 16:27
  • 1
    \$\begingroup\$ You only use it iff from_chars signals success. Leaving it uninitialized might marginally increase efficiency, or might change nothing, depending on the compiler. \$\endgroup\$ Jan 29 at 16:43
  • 1
    \$\begingroup\$ cv-qualifiers on function-parameters have no impact on function signature. Thus, they are only relevant for the definition, where they only impact the body (and ctor-init-list for ctors). \$\endgroup\$ Jan 29 at 18:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.