[libc-commits] [PATCH] D107792: [libc] add strtoll function and backend

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Just one major comment about `errno` but the rest are mostly nits and bikeshed comments.



================
Comment at: libc/src/__support/CMakeLists.txt:16
+add_header_library(
+  stdlib_utils
+  HDRS
----------------
Bikeshed: The name `stdlib_utils` sounds very general and can end up with more than one topic. May be call it `str_conv_utils` to keep it focused.


================
Comment at: libc/src/__support/CMakeLists.txt:20
+  DEPENDS
+    libc.src.__support.ctype_utils
+    libc.include.errno
----------------
For targets in the same directory, you use `.ctype_utils`.


================
Comment at: libc/src/__support/stdlib_utils.h:12
+
+#include "include/errno.h"
+#include "src/__support/ctype_utils.h"
----------------
We don't include LLVM libc's errno now. Just do `#include <errno.h>` and refer to `errno` directly in code. Lint rules will catch if the header file is not from LLVM libc. For there reference to `errno` in code, you might need a `NOLINT` comment.

If you do the standard build, as in not the full build, my feeling is this will not build.


================
Comment at: libc/src/__support/stdlib_utils.h:39
+// it. This will advance the string pointer.
+static inline int base_from_num_start(const char *__restrict *__restrict src) {
+  if (**src == '0') {
----------------
May be `s/base_from_num_start/infer_base` ?


================
Comment at: libc/src/__support/stdlib_utils.h:41
+  if (**src == '0') {
+    ++*src;
+    if ((**src | 32) == 'x') {
----------------
Nit: Format it with parenthesis: `++(*src)`?


================
Comment at: libc/src/__support/stdlib_utils.h:55
+// the other functions.
+static inline long long llong_from_string(const char *__restrict src,
+                                          char **__restrict str_end, int base) {
----------------
Since this is in an internal namespace, you can name it `strtoll`.


================
Comment at: libc/src/__support/stdlib_utils.h:78
+
+  unsigned long long const CUR_LLONG_MAX =
+      (result_sign == '+' ? LLONG_MAX
----------------
Nit: What does the `CUR` prefix indicate? It seems to indicate the current value of some variable being updated in a loop. But it is actually a `const`.


================
Comment at: libc/test/src/stdlib/strtoll_test.cpp:53
+  ASSERT_THAT(__llvm_libc::strtoll(too_big_number, &str_end, 10),
+              Fails(ERANGE, __LONG_LONG_MAX__));
+  EXPECT_EQ(str_end - too_big_number, 19l);
----------------
Why not `LLONG_MAX`?


================
Comment at: libc/test/src/stdlib/strtoll_test.cpp:58
+  ASSERT_THAT(__llvm_libc::strtoll(too_big_negative_number, &str_end, 10),
+              Fails(ERANGE, (-__LONG_LONG_MAX__ - 1)));
+  EXPECT_EQ(str_end - too_big_negative_number, 20l);
----------------
`LLONG_MIN` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107792