Validating an integer with affixes in compile-time or runtime

Question

I'm implementing a function called is_proper_integer that will check the string whether it is a valid integer literal by borrowing the idea from C++ grammar.

Function signature:

constexpr bool is_proper_integer(std::string_view str, 
                                 std::optional<std::reference_wrapper<parse_integer_errc_t>> errc = {});

Where:

• the function is declared as constexpr so that the function is usable at compile-time.
• str has the plain type std::string_view so that I could modify it by calling remove_prefix and remove_suffix within the definition.
• errc acts as an error code to report the reason why it returns false and I made it std::optional<std::reference_wrapper<...>> because the parameter is obviously optional and there is no std::optional<T&>, so I wrap it inside the class template std::reference_wrapper.

These are the helpful utilities in order to work with the function:

enum class parse_integer_errc_t {
    normal,
    foreign_char,
    adjacent_radix_sep,
    invalid_prefix,
    invalid_radix_sep,
    invalid_binary_fmt,
    invalid_octal_fmt,
    invalid_decimal_fmt,
    invalid_hexadecimal_fmt,
    unknown
};

enum class integer_prefix_t {
    binary, octal, decimal, hexadecimal, unknown
};

std::ostream& operator<<(std::ostream& os, parse_integer_errc_t errc) {
    switch (errc) {
        using enum parse_integer_errc_t;
        case normal:                  return os << "normal";
        case foreign_char:            return os << "foreign_char";
        case adjacent_radix_sep:      return os << "adjacent_radix_sep";
        case invalid_prefix:          return os << "invalid_prefix";
        case invalid_radix_sep:       return os << "invalid_radix_sep";
        case invalid_binary_fmt:      return os << "invalid_binary_fmt";
        case invalid_octal_fmt:       return os << "invalid_octal_fmt";
        case invalid_decimal_fmt:     return os << "invalid_decimal_fmt";
        case invalid_hexadecimal_fmt: return os << "invalid_hexadecimal_fmt";
        case unknown:                 return os << "unknown";
    }
    return os;
}

Main implementation:

constexpr bool is_proper_integer(std::string_view str,
                                 std::optional<std::reference_wrapper<parse_integer_errc_t>> errc = {}) {

    constexpr char valid_chars[] = {
        '0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
        'A', 'B', 'C', 'D', 'E', 'F',
        'a', 'b', 'c', 'd', 'e', 'f'
    };

    /// span version
    auto&& span_chars = std::span{valid_chars};

    /// check if it has unary '-' or '+' operator
    if (str.front() == '-' || str.front() == '+') str.remove_prefix(1);

    /// check if the front character is valid (after '-')
    if (
        auto&& span_ = span_chars.first<10>();
        std::ranges::find(span_, str.front()) == span_.end()
    ) {
        if (errc.has_value()) errc->get() = parse_integer_errc_t::invalid_prefix;
        return false;
    }

    /// check the prefix
    integer_prefix_t flag_ = integer_prefix_t::unknown;

    if (str.starts_with("0x") | str.starts_with("0X")) {
        str.remove_prefix(2);
        flag_ = integer_prefix_t::hexadecimal;
    } else if (str.starts_with("0b") || str.starts_with("0B")) {
        str.remove_prefix(2);
        flag_ = integer_prefix_t::binary;
    } else if (str.starts_with('0')) {
        str.remove_prefix(1);
        flag_ = integer_prefix_t::octal;
    } else {
        flag_ = integer_prefix_t::decimal;
    }

    /// check again...
    if (str.starts_with('\'')) {
        if (errc.has_value()) errc->get() = parse_integer_errc_t::invalid_radix_sep;
        return false;
    }

    /// check the suffix
    if (str.ends_with("ull") ||
        str.ends_with("ULL") ||
        str.ends_with("uLL") ||
        str.ends_with("Ull")
    ) str.remove_suffix(3);
    else if (
        str.ends_with("ul") ||
        str.ends_with("uL") ||
        str.ends_with("Ul") ||
        str.ends_with("UL") ||
        str.ends_with("ll") ||
        str.ends_with("LL") ||
        str.ends_with("uz") ||
        str.ends_with("uZ") ||
        str.ends_with("Uz") ||
        str.ends_with("UZ")
    ) str.remove_suffix(2);
    else if (
        str.ends_with('z') ||
        str.ends_with('Z') ||
        str.ends_with('u') ||
        str.ends_with('U') ||
        str.ends_with('l') ||
        str.ends_with('L')
    ) str.remove_suffix(1);

    /// check again...
    if (str.ends_with('\'')) {
        if (errc.has_value()) errc->get() = parse_integer_errc_t::invalid_radix_sep;
        return false;
    }

    /// flag for radix grouping check
    bool radix_sep_state { false };

    /// normal loop checking
    for (char c : str) {
        if (c == '\'') {
            if (radix_sep_state) {
                if (errc.has_value()) errc->get() = parse_integer_errc_t::adjacent_radix_sep;
                return false;
            }
            radix_sep_state = true;
            continue;
        }
        radix_sep_state = false;
        if (std::ranges::find(span_chars, c) == span_chars.end()) {
            if (errc.has_value()) errc->get() = parse_integer_errc_t::foreign_char;
            return false;
        }
        switch (flag_) {
            using enum integer_prefix_t;
            case binary: {
                auto&& span_ = span_chars.first<2>();
                if (std::ranges::find(span_, c) == span_.end()) {
                    if (errc.has_value()) errc->get() = parse_integer_errc_t::invalid_binary_fmt;
                    return false;
                }
                break;
            }
            case octal: {
                auto&& span_ = span_chars.first<8>();
                if (std::ranges::find(span_, c) == span_.end()) {
                    if (errc.has_value()) errc->get() = parse_integer_errc_t::invalid_octal_fmt;
                    return false;
                }
                break;
            }
            case decimal: {
                auto&& span_ = span_chars.first<10>();
                if (std::ranges::find(span_, c) == span_.end()) {
                    if (errc.has_value()) errc->get() = parse_integer_errc_t::invalid_decimal_fmt;
                    return false;
                }
                break;
            }
            case hexadecimal: {
                if (std::ranges::find(span_chars, c) == span_chars.end()) {
                    if (errc.has_value()) errc->get() = parse_integer_errc_t::invalid_hexadecimal_fmt;
                    return false;
                }
                break;
            }
            case unknown: {
                if (errc.has_value()) errc->get() = parse_integer_errc_t::unknown;
                return false;
            }
        }
    }

    if (errc.has_value()) errc->get() = parse_integer_errc_t::normal;

    return true;
}

Compile-time Check:

    static_assert(is_proper_integer("1234")); /// normal case
    static_assert(is_proper_integer("+1234")); /// positive
    static_assert(is_proper_integer("-1234")); /// negative
    static_assert(is_proper_integer("1234u")); /// has suffix 'u'
    static_assert(is_proper_integer("-1234ull")); /// has suffix 'ull' (ofc, overflow, but still valid)
    static_assert(is_proper_integer("1234uz")); /// has suffix 'uz'
    static_assert(is_proper_integer("0b10010")); /// ok, binary
    static_assert(!is_proper_integer("0b1012")); /// '2' strayed in binary fmt
    static_assert(is_proper_integer("+0B11001U")); /// has suffix 'U'
    static_assert(!is_proper_integer("ajsdsad")); /// obviously, invalid chars
    static_assert(is_proper_integer("0172613")); /// octal, ok
    static_assert(is_proper_integer("-012112")); /// ok
    static_assert(is_proper_integer("+0xabc'def")); /// hexadecimal, ok
    static_assert(is_proper_integer("-0XCAFEdead")); /// ok, case-insensitive
    static_assert(is_proper_integer("0xdeadbeefull")); /// ok, has suffix 'ull'
    static_assert(!is_proper_integer("deadbeef")); /// invalid prefix
    static_assert(!is_proper_integer("12'3''2")); /// wrong radix grouping.
    static_assert(is_proper_integer("0b1111'0000'1010ull")); /// ok
    static_assert(!is_proper_integer("0123;1")); /// foreign character

Runtime Check:

    std::vector<std::string_view> queues {
        "1234", "+1234", "-1234", "1234u",
        "-1234ull", "1234uz", "0b10010",
        "0b1012", "+0B11001U", "ajsdsad",
        "0172613", "-012112", "+0xabc'def",
        "-0XCAFEdead", "0xdeadbeefull", "deadbeef",
        "12'3''2", "0b1111'0000'1010ull", "0123;1"
    };
    parse_integer_errc_t errcode;

    std::cout << std::boolalpha;

    for (const auto& str : queues) {
        std::cout
        << "String: "    << std::setw(23) << std::left << str
        << "Is valid?: " << std::setw(8)  << std::left
        << (is_proper_integer(str, errcode) ? "yes" : "no")
        << "Reason: "    << std::setw(20) << std::left << errcode << '\n';
    }

Output:

String: 1234                   Is valid?: yes     Reason: normal
String: +1234                  Is valid?: yes     Reason: normal
String: -1234                  Is valid?: yes     Reason: normal
String: 1234u                  Is valid?: yes     Reason: normal
String: -1234ull               Is valid?: yes     Reason: normal
String: 1234uz                 Is valid?: yes     Reason: normal
String: 0b10010                Is valid?: yes     Reason: normal
String: 0b1012                 Is valid?: no      Reason: invalid_binary_fmt
String: +0B11001U              Is valid?: yes     Reason: normal
String: ajsdsad                Is valid?: no      Reason: invalid_prefix
String: 0172613                Is valid?: yes     Reason: normal
String: -012112                Is valid?: yes     Reason: normal
String: +0xabc'def             Is valid?: yes     Reason: normal
String: -0XCAFEdead            Is valid?: yes     Reason: normal
String: 0xdeadbeefull          Is valid?: yes     Reason: normal
String: deadbeef               Is valid?: no      Reason: invalid_prefix
String: 12'3''2                Is valid?: no      Reason: adjacent_radix_sep
String: 0b1111'0000'1010ull    Is valid?: yes     Reason: normal
String: 0123;1                 Is valid?: no      Reason: foreign_char

Is there anything I could simplify?

Answer 1

Is there anything I could simplify?

I’m going to answer this question specifically, rather than give a general code review. And they answer is: yes… this function is way too big, and way too complex. This calls for refactoring.

Merely breaking the function into smaller logical chunks would be good enough on its own; nobody wants to maintain a 100 line function (or even a 50 line function; if your function is longer than 10 lines, that’s a smell; if your single function creates a scroll bar on CodeReview, that’s really a smell). But there are easily half a dozen things this function does that would be useful on their own. Breaking them out would not only benefit future projects, it would allow those smaller functions to be brutally tested individually, which would be better for is_proper_integer() too. (It would, for example, probably have immediately caught the bug in the sign check part.)

For example:

auto has_proper_integer_sign(std::string_view);
auto has_proper_integer_prefix(std::string_view);
auto has_proper_integer_suffix(std::string_view);
auto has_proper_integer_binary_digits(std::string_view);
auto has_proper_integer_octal_digits(std::string_view);
auto has_proper_integer_decimal_digits(std::string_view);
auto has_proper_integer_hexadecimal_digits(std::string_view);

With functions like these, the main function is much easier to write and understand:

auto is_proper_integer(std::string_view s)
{
    return has_proper_integer_sign(s)
        and has_proper_integer_prefix(s)
        and has_proper_integer_suffix(s)
        // and so on...
}

This alone isn’t really that much help, which is why I would also suggest refactoring each test into a function that actually parses. For example:

enum class integer_parse_errc
{
    // all the error codes you'll need for parsing integers
};

// of course, add all the machinery to make the above a legit system_error
// error code (like a category, is_error_code_enum, etc.)

enum class integer_sign_type
{
    none,
    positive,
    negative
};

struct parse_integer_sign_result_t
{
    integer_sign_type type = integer_sign_type::none;
    std::string_view sign = {};
    std::string_view rest = {};
    integer_parse_errc ec = {};
};

constexpr auto parse_integer_sign(std::string_view s) noexcept -> parse_integer_sign_result_t
{
    if (not s.empty())
    {
        auto const c = s.front();
        auto const sign = s.substr(0, 1);
        auto const rest = s.substr(1);

        if (c == '+')
            return {integer_sign_type::positive, sign, rest};
        if (c == '-')
            return {integer_sign_type::negative, sign, rest};
    }

    // or maybe you could consider being empty an error condition
    //
    // that's up to you

    return {integer_sign_type::none, {}, s};
}

Now your function can look something like:

constexpr auto is_proper_integer(std::string_view s)
{
    // First parse the sign.
    if (auto const res = parse_integer_sign(s); res.ec != integer_parse_errc{})
        return false;
    else
        s = res.rest;

    // Now parse the prefix
    if (auto const res = parse_integer_prefix(s); res.ec != integer_parse_errc{})
        return false;
    else
        s = res.rest;

    // ... and so on
}

Even better, you could probably write a function that parses an integer into its components, and then base is_proper_integer() on that:

struct parse_integer_result_t
{
    // the parsed components:
    sign_t sign     = sign_t::none;         // or positive/negative
    prefix_t prefix = prefix_t::decimal;    // or binary/octal/hexadecimal
    suffix_t suffix = suffix_t::none;       // or l/ll/z/u/ul/ull/uz

    // the unparsed components:
    std::string_view sign_part = {};
    std::string_view prefix_part = {};
    std::string_view suffix_part = {};
    std::string_view number_part = {};

    integer_parse_errc = {};
};

constexpr auto parse_integer_components(std::string_view s) noexcept
{
    // for each component XXX, basically do:
    //     auto XXX_res = parse_integer_XXX(s);
    // whittling down the string as you go

    // if any return an error code, you're done, just return the error code

    // if none of them return an error code, construct the full parse result
    // from the component results you already have
}

constexpr auto is_proper_integer(std::string_view s) noexcept
{
    return parse_integer_components(s).ec == integer_parse_errc{};
}

The parse_integer_components() function would be useful in its own right, because you could then take the information in the components and actually parse the number if you want, or just do stuff like make sure the number is non-negative, or in hex form, or whatever you please.

That’s not all you could refactor out of this function. Another bit of functionality you use repeatedly that would also be useful on its own is caseless comparisons. For example, you have:

    if (str.ends_with("ull") ||
        str.ends_with("ULL") ||
        str.ends_with("uLL") ||
        str.ends_with("Ull")

That’s not just ugly, it’s incomplete. You forgot llu, LLu, llU, and LLU.

Wouldn’t it be much nicer to write:

    if (ends_with_ci(str, "ull") or ends_with_ci(str, "llu"))

And ends_with_ci() is a function you could use many other places, too.

It might make sense to write:

constexpr auto starts_with_ci(std::string_view, char) noexcept -> bool;
constexpr auto starts_with_ci(std::string_view, std::string_view) noexcept -> bool;

constexpr auto ends_with_ci(std::string_view, char) noexcept -> bool;
constexpr auto ends_with_ci(std::string_view, std::string_view) noexcept -> bool;

constexpr auto equals_ci(char, char) noexcept -> bool;
constexpr auto equals_ci(std::string_view, char) noexcept -> bool;
constexpr auto equals_ci(char, std::string_view) noexcept -> bool;
constexpr auto equals_ci(std::string_view, std::string_view) noexcept -> bool;

// and maybe a find function that returns the found position or end,
// and a contains function that just uses the find function and returns bool.
//
// maybe also a rfind function.
//
// up to you

These functions make your parse functions much simpler, and have other uses too.

Here’s a summary of suggestions I would make:

1. Refactor EVERYTHING. Find the bits that are reusable, and useful on their own, and pull them out into their own functions. That includes not only the parsing sub-processes (parsing signs, parsing prefixes, parsing suffixes, etc.), but also algorithms you use throughout (caseless comparisons). And, of course, test everything rigorously. Testing smaller components of bigger functions makes you more confident about the bigger functions.
2. Don’t use out parameters; no not even optional out parameters. Here I strongly disagree with @JDługosz: do NOT copy what <filesystem> did. That was a disaster only created because we didn’t have std::expected or std::error or structured bindings… don’t repeat the mistake. Return result structs for any functions that return anything more than one “thing”. For any parse functions, that will be at least what was parsed, the remainder that was not parsed (so you can continue parsing with other functions), and an error code if there was a parse failure. Returning structs is much more ergonomic than out parameters—even optional out parameters—especially with structured bindings.
3. Don’t return unnecessary information. If your function is is_proper_integer(), then it needs only return bool. It does not need to return an error code (and certainly not an optional-reference-error-code… which, by the way, should really just be parse_integer_errc_t* ec = nullptr (though, even better, should be two different functions, because optional arguments suck), and you should do if (ec != nullptr) *ec = whatever-error;). If the question is “is it an integer?”, that is a yes-or-no question. If the question is “why is it not an integer?” that is not the same question as “is it an integer?”, so it should be a different function. is_proper_integer() answers the yes-or-no question; parse_integer_components() answers the more complex question, with all the detail you could want.

Answer 2

the function is declared as constexpr so that the function is usable at compile-time.

good!

str has the plain type std::string_view so that I could modify it by calling remove_prefix and remove_suffix within the definition.

good! that's a good feature for this kind of parsing.

errc acts as an error code to report the reason why it returns false and I made it std::optionalstd::reference_wrapper<...> because the parameter is obviously optional and there is no std::optional<T&>, so I wrap it inside the class template std::reference_wrapper.

You might consider two overloaded forms, with and without an error_code& out parameter, as seen with the <filesystem> header. The form without is just a one-liner that passes a local variable to the regular form, essentially throwing it away. There's no complicated code in the function for dealing with an optional.

standard error codes

You should define an error category class that has the proper function for looking up the error message, rather than the operator<< you have.

Your stand-alone parse_integer_errc_t should be able to be set as error_code.

repeated calls

You have a long list of .ends_with(xxx) with different strings. You could put all the allowed suffixes in a simple array and use a loop, which also picks up the length of the suffix automatically rather than needing different cases.

If you do a check that folds case rather than .ends_with you would not need a power-of-two of the length of the suffix for the number of cases to try!

 constexpr string_view suffixes[]= {"ull","ul","l","uz","z","u"};
 for (auto sfx : suffixes) {
      if case-insentive-match of the end of the input,
          remove the suffix and break
 }

alternatives

A much simpler (but probably slower) way to check would be to use a regex. There is a Compile Time RegEx library (work in progress) that is looking quite nice, that would let you check compile-time strings.

Or, you could just call from_chars and see if you got an error or not, ignoring the parsed integer it produced.

Answer 3

In no particular order:

• enum class parse_integer_errc_t { normal, ... I think if we call this an error code (errc), then no_error would be a clearer name than normal. On the other hand, if the intent is to categorise the string, then the enum shouldn't be referred to as an "error code".

• unknown I think this category could be removed (both in the error code and the prefix type).

• It's a lot simpler to always require a parse_integer_errc_t& parameter. We could also provide an overloaded version taking only one parameter, that calls the first one and discards the error code internally.

• auto&& span_chars = std::span{valid_chars}; The && is unnecessary (as in other declarations below).

• bug: if (str.front() == '-' || str.front() == '+') str.remove_prefix(1); We need to check that the string isn't empty first!!! (Even for std::string this is undefined behavior for an empty string, and std::string_view doesn't have the same guarantees of a terminating null).

• str.ends_with("ull") I guess it depends what format spec you're following, but llu (and case variations) are also valid for C++ literals: https://en.cppreference.com/w/cpp/language/integer_literal

•       auto&& span_ = span_chars.first<2>();
        if (std::ranges::find(span_, c) == span_.end()) {
            if (errc.has_value()) errc->get() = parse_integer_errc_t::invalid_binary_fmt;
            return false;
        }
  

  If we define the span of valid characters (and the error code) outside of the loop, we shouldn't need the switch statement and the duplication here.