[libc-commits] [PATCH] D107999: [libc] change internal strtoll to be templated

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

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).



================
Comment at: libc/src/__support/str_conv_utils.h:78-81
+  unsigned long long const NEGATIVE_MAX =
+      (__llvm_libc::cpp::NumericLimits<T>::min() != 0)
+          ? __llvm_libc::cpp::NumericLimits<T>::min()
+          : __llvm_libc::cpp::NumericLimits<T>::max();
----------------
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();
``` 


================
Comment at: libc/src/__support/str_conv_utils.h:111
   else
-    return -result;
+    return -(T)(result);
 }
----------------
We should prefer `T(...)` or `static_cast<T>(...)` over c-style casts. Also, use the ternary operator instead of an `if`-`else` statement?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107999