Updates the internal string conversion function so that it
uses the new Limits.h added in a previous commit for max and min values,
and has a templated type. This makes implementing the other strto*
functions very simple.
Details
- Reviewers
sivachandra - Commits
- rGd52f0aeca5db: [libc] Add strtol, strtoul, and strtoull
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Can you add implementation of strtol and strtoul and tests for them in this same patch? They will help us catch problems in the corner cases if any (one of which I have commented about inline).
libc/src/__support/str_conv_utils.h | ||
---|---|---|
78–81 | I am not sure this is doing what you want. Lets take the example of T being the type int. I think you want NEGATIVE_MAX to be INT_MAX + 1. But, the above code is effectively: unsigned long long const NEGATIVE_MASK = INT_MIN; Which makes NEGATIVE_MASK a number much larger than INT_MAX + 1. This should fix it: unsigned long long const NEGATIVE_MAX = (__llvm_libc::cpp::NumericLimits<T>::min() != 0) ? static_cast<unsigned long long>(__llvm_libc::cpp::NumericLimits<T>::max()) + 1 : __llvm_libc::cpp::NumericLimits<T>::max(); | |
114–131 | We should prefer T(...) or static_cast<T>(...) over c-style casts. Also, use the ternary operator instead of an if-else statement? |
libc/src/__support/str_conv_utils.h | ||
---|---|---|
109–110 | There is change in behavior from the earlier code. Even after detecting that the result is out of range we advance the pointer now, which was not the case previously because of the break statements. But I think this change is doing it right - it is reading out the full number even if it is out of range. So, may be add a test for it? Also, can we reduce the loop body once overflow is detected? As in, before line 92, can we have something like: if (result == ABS_MAX) continue; | |
124 | When the sign is negative and result is ABS_MAX and the return type is a signed integer type, then I think we have to handle that specially. Consider this: T is long, result_sign is - and result is ABS_MAX which is effectively LONG_MAX + 1. Since LONG_MAX + 1 cannot be represent as a long type, the static_cast here becomes an implementation defined operation. I think previously also it was implementation defined. | |
libc/test/src/stdlib/strtoll_test.cpp | ||
170 | Why did you change this to unsigned here and other similar places? |
fix the static cast, add a test for long numbers, and a shortcut for the loop.
libc/src/__support/str_conv_utils.h | ||
---|---|---|
109–110 | Ah, yes the change in behavior was intentional, I noticed that the str_end value was different between my implementation and the existing strtoll. I have added a test to check that in all of the strto* functions. I also added the loop shortcut. | |
124 | You're right, I've added an extra case and a comment explaining it. | |
libc/test/src/stdlib/strtoll_test.cpp | ||
170 | I changed it because the compiler was giving warnings for comparing int base with the unsigned digits (e.g. first_digit < base) and so I made it unsigned, and decided to make it consistent across all of the strto* tests. It doesn't affect anything. |
libc/src/__support/str_conv_utils.h | ||
---|---|---|
78 | This can be constexpr. | |
94 | Add a comment here explaining the early return; point out that we read out the full number even if it is out of range. | |
124 | Can we may be handle the overflow case like this: if (result == ABS_MAX) { if (result_sign == '+' || is_unsigned) return __llvm_libc::cpp::NumericLimits<T>::max(); else // T is signed and there is a negative overflow return __llvm_libc::cpp::NumericLimits<T>::min(); } For better readability, before line 78 you can define the following and use it above and also on line 79. constexpr bool is_unsigned = (__llvm_libc::cpp::NumericLimits<T>::min() == 0); Along the same lines, you can probably also define: const bool is_positive = (result_sign == '+'); | |
libc/src/stdlib/CMakeLists.txt | ||
93 | Fix here and in many other files. |
cleaned up over/underflow handling. Fixed an edge case with passing LLONG_MAX followed by other numbers to strtoll.
libc/src/__support/str_conv_utils.h | ||
---|---|---|
94 | I did that, in addition I added setting errno, since it's possible to hit ABS_MAX exactly, and then have more numbers. With the code as it was previously that would not set ERANGE in that case. I've added a test to strtoll_test.cpp to handle that. |
small tweaks, respond to comments.
libc/src/__support/str_conv_utils.h | ||
---|---|---|
94 | Thank you, and we can't do that because that would set errno if the input is exactly the maximum as well as if it's greater than the maximum. |
libc/src/__support/str_conv_utils.h | ||
---|---|---|
94 | Ah, ok! |
This can be constexpr.