-
Notifications
You must be signed in to change notification settings - Fork 12
add numeric conversion stoi #43
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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:
-
Use
const_iterator
type:
Instead of usingauto
for the iterator types, explicitly specify them astypename fixed_string<N>::const_iterator
. This makes the code more readable and easier to understand. -
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. -
Utilize
std::isspace
overload:
Instead of comparing the result ofstd::isspace(*it)
with0
, you can directly use the result in the condition. Thestd::isspace
function returns a non-zero value if the character is a whitespace character. -
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. -
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. -
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;
}
auto it = str.cbegin(); | ||
auto ite = str.cend(); |
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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
No description provided.