Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add numeric conversion stoi #43

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

abel-mak
Copy link

No description provided.

Copy link
Owner

@unterumarmung unterumarmung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for the delay in reviewing your code. I must say, the code appears to be rather intricate and challenging to comprehend. It would be immensely helpful if you could consider refactoring it to enhance its structure and make it more maintainable. Additionally, I kindly request that you thoroughly test each segment of the function to ensure its proper functionality. This way, we can be confident that it behaves correctly. Thank you for your efforts thus far.

Here you can see the ChatGPT review of your code, please take it with a grain of salt.


Here are some suggestions to improve the stoi function:

  1. Use const_iterator type:
    Instead of using auto for the iterator types, explicitly specify them as typename fixed_string<N>::const_iterator. This makes the code more readable and easier to understand.

  2. Validate input before processing:
    Add an early validation step to check if the base argument is within the valid range of 2 to 36. If it's outside this range, you can throw an exception or return an appropriate error value.

  3. Utilize std::isspace overload:
    Instead of comparing the result of std::isspace(*it) with 0, you can directly use the result in the condition. The std::isspace function returns a non-zero value if the character is a whitespace character.

  4. Separate the parsing logic into helper functions:
    Split the parsing logic into smaller helper functions to improve readability and maintainability. For example, you can have separate functions to handle sign parsing, prefix parsing (for hexadecimal numbers), and digit parsing.

  5. Use range-based for loop (C++11 or later):
    Instead of using iterators explicitly, you can use a range-based for loop to iterate over the characters in the string. This simplifies the code and makes it more concise.

  6. Consider using std::from_chars (C++17 or later):
    In C++17, the std::from_chars function was introduced, which can parse integers from strings with better performance than the standard library functions. If you're targeting a C++17 or later environment, you might consider using this function instead.

Taking these suggestions into account, here's an updated version of the stoi function:

template <size_t N>
constexpr int stoi(const fixed_string<N>& str, int base = 10)
{
    static_assert(N > 0, "Empty fixed_string");

    if (base < 2 || base > 36)
    {
        throw std::invalid_argument("Invalid base");
    }

    using const_iterator = typename fixed_string<N>::const_iterator;
    const_iterator it = str.cbegin();
    const_iterator ite = str.cend();

    // Skip leading whitespace
    while (it != ite && std::isspace(*it))
    {
        ++it;
    }

    int sign = 1;

    // Parse sign
    if (it != ite && (*it == '-' || *it == '+'))
    {
        sign = (*it == '-') ? -1 : 1;
        ++it;
    }

    // Parse prefix for hexadecimal numbers
    if (base == 16 && std::distance(it, ite) >= 2 && *it == '0' && std::tolower(*(it + 1)) == 'x')
    {
        it += 2;
    }

    int res = 0;

    // Helper function to determine the value of a digit
    auto get_digit_value = [](char c) -> int {
        if (std::isdigit(c))
        {
            return c - '0';
        }
        else if (std::isalpha(c))
        {
            return std::tolower(c) - 'a' + 10;
        }
        else
        {
            return -1; // Invalid character
        }
    };

    // Parse digits
    for (; it != ite; ++it)
    {
        const int tmp = res;
        int digit_value = get_digit_value(*it);

        if (digit_value >= 0 && digit_value < base)
        {
            res = (res * base) + (digit_value * sign);

            if ((sign > 0 && tmp > res) || (sign < 0 && tmp < res))
            {
                throw std::out_of_range("fixedstr stoi");
            }
        }
        else
        {
            break; // Invalid character or out of range digit
        }
    }

    return res;
}

Comment on lines +621 to +622
auto it = str.cbegin();
auto ite = str.cend();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, use more descriptive variable names

else
break;
if ((sign > 0 && tmp > res) || (sign < 0 && tmp < res))
throw std::out_of_range("fixedstr stoi");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the error name should be more descriptive, something like: "FIXSTR_PRETTY_FUNCTION + ": the value " + str + " is out of range"

Where FIXSTR_PRETTY_FUNCTION is defined like this (pseudocode):

#ifdef _MSC_VER
    #define FIXSTR_PRETTY_FUNCTION fixstr::fixed_string{__FUNCSIG__}
#else // clang, gcc and others
    #define FIXSTR_PRETTY_FUNCTION fixstr::fixed_string{__PRETTY_FUNCTION__}
#endif

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants