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

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


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


================
Comment at: libc/src/__support/str_conv_utils.h:120
+  return (result_sign == '+') ? static_cast<T>(result)
+                              : -static_cast<T>(result);
 }
----------------
sivachandra wrote:
> 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.
> 
You're right, I've added an extra case and a comment explaining it.


================
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) {
----------------
sivachandra wrote:
> Why did you change this to `unsigned` here and other similar places?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107999