[libc] Add strtol, strtoul, and strtoull

[michaelrj]

Summary

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.

[sivachandra]

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();

111

We should prefer T(...) or static_cast<T>(...) over c-style casts. Also, use the ternary operator instead of an if-else statement?

[michaelrj]

add strtol, strtoul, and strtoull, as well as address comments.

[sivachandra]

libc/src/__support/str_conv_utils.h

103

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;

121

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

162 ↗

(On Diff #366365)

Why did you change this to unsigned here and other similar places?

[michaelrj]

fix the static cast, add a test for long numbers, and a shortcut for the loop.

libc/src/__support/str_conv_utils.h

103

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.

121

You're right, I've added an extra case and a comment explaining it.

libc/test/src/stdlib/strtoll_test.cpp

162 ↗

(On Diff #366365)

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.

[sivachandra]

libc/src/__support/str_conv_utils.h

78

This can be constexpr.

92

Add a comment here explaining the early return; point out that we read out the full number even if it is out of range.

121

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 ↗

(On Diff #366761)

Fix here and in many other files.

[michaelrj]

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

92

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.

[sivachandra]

libc/src/__support/str_conv_utils.h

92

Good catch! You can probably move setting of errno from here (and below) to inside the conditional at line 120.

119

You don't need parenthesis around is_positive.

[michaelrj]

small tweaks, respond to comments.

libc/src/__support/str_conv_utils.h

92

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.

[sivachandra]

libc/src/__support/str_conv_utils.h

92

Ah, ok!

libc/src/__support/CMakeLists.txt

 add_header_library(
   str_conv_utils
   HDRS
     str_conv_utils.h
   DEPENDS
     .ctype_utils
     libc.include.errno
     libc.src.errno.__errno_location
+    libc.utils.CPP.standalone_cpp
 )
 
 add_header_library(
   integer_operations
   HDRS
     integer_operations.h
 )
 
 add_subdirectory(FPUtil)

libc/src/__support/str_conv_utils.h

 //===-- Stdlib utils --------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LIBC_SRC_STDLIB_STDLIB_UTILS_H
 #define LIBC_SRC_STDLIB_STDLIB_UTILS_H
 
 #include "src/__support/ctype_utils.h"
+#include "utils/CPP/Limits.h"
 #include <errno.h>
 #include <limits.h>
 
 namespace __llvm_libc {
 namespace internal {
 
 // Returns a pointer to the first character in src that is not a whitespace
 // character (as determined by isspace())
if (**src == '0') {
     return 8;
   }
   return 10;
 }
 
 // Takes a pointer to a string, a pointer to a string pointer, and the base to
 // convert to. This function is used as the backend for all of the string to int
 // functions.
-static inline long long strtoll(const char *__restrict src,
-                                char **__restrict str_end, int base) {
+template <class T>
+static inline T strtointeger(const char *__restrict src,
   unsigned long long result = 0;
 
   if (base < 0 || base == 1 || base > 36) {
     errno = EINVAL; // NOLINT
     return 0;
   }
 
   src = first_non_whitespace(src);
 
   char result_sign = '+';
   if (*src == '+' || *src == '-') {
     result_sign = *src;
     ++src;
   }
 
   if (base == 0) {
     base = infer_base(&src);
   } else if (base == 16 && *src == '0' && (*(src + 1) | 32) == 'x') {
     src = src + 2;
   }
 
+  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();
   unsigned long long const ABS_MAX =
-      (result_sign == '+' ? LLONG_MAX
-                          : static_cast<unsigned long long>(LLONG_MAX) + 1);
+      (result_sign == '+' ? __llvm_libc::cpp::NumericLimits<T>::max()
+                          : NEGATIVE_MAX);
   unsigned long long const ABS_MAX_DIV_BY_BASE = ABS_MAX / base;
   while (isalnum(*src)) {
     int cur_digit = b36_char_to_int(*src);
     if (cur_digit >= base)
       break;
     if (result > ABS_MAX_DIV_BY_BASE) {
       result = ABS_MAX;
       errno = ERANGE; // NOLINT
       break;
     }
     result = result * base;
     if (result > ABS_MAX - cur_digit) {
       result = ABS_MAX;
       errno = ERANGE; // NOLINT
       break;
     }
     result = result + cur_digit;
 
     ++src;
   }
 
   if (str_end != nullptr)
     *str_end = const_cast<char *>(src);
   if (result_sign == '+')
-    return result;
+    return (T)(result);
   else
-    return -result;
+    return -(T)(result);
 }
 
 } // namespace internal
 } // namespace __llvm_libc
 
 #endif // LIBC_SRC_STDLIB_STDLIB_UTILS_H

libc/src/stdlib/strtoll.cpp

 #include "src/__support/common.h"
 #include "src/__support/str_conv_utils.h"
 
 namespace __llvm_libc {
 
 LLVM_LIBC_FUNCTION(long long, strtoll,
                    (const char *__restrict str, char **__restrict str_end,
                     int base)) {
-  return internal::strtoll(str, str_end, base);
+  return internal::strtointeger<long long>(str, str_end, base);
 }
 
 } // namespace __llvm_libc