[libc-commits] [PATCH] D107999: [libc] Add strtol, strtoul, and strtoull

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/__support/str_conv_utils.h:105
 
     ++src;
   }
----------------
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;
```


================
Comment at: libc/src/__support/str_conv_utils.h:120
+  return (result_sign == '+') ? static_cast<T>(result)
+                              : -static_cast<T>(result);
 }
----------------
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.



================
Comment at: libc/test/src/stdlib/strtoll_test.cpp:162
   char small_string[4] = {'\0', '\0', '\0', '\0'};
-  for (int base = 2; base <= 36; ++base) {
+  for (unsigned int base = 2; base <= 36; ++base) {
     for (long long first_digit = 0; first_digit <= 36; ++first_digit) {
----------------
Why did you change this to `unsigned` here and other similar places?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107999/new/

https://reviews.llvm.org/D107999