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

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added inline comments.
This revision is now accepted and ready to land.


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


================
Comment at: libc/src/__support/str_conv_utils.h:78
 
+  unsigned long long const NEGATIVE_MAX =
+      (__llvm_libc::cpp::NumericLimits<T>::min() != 0)
----------------
This can be `constexpr`.


================
Comment at: libc/src/__support/str_conv_utils.h:94
+    ++src;
+
+    if (result == ABS_MAX)
----------------
Add a comment here explaining the early return; point out that we read out the full number even if it is out of range.


================
Comment at: libc/src/stdlib/CMakeLists.txt:93
+)
\ No newline at end of file

----------------
Fix here and in many other files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107999