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

Siva Chandra via Phabricator via libc-commits libc-commits at lists.llvm.org
Tue Aug 10 10:28:32 PDT 2021

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
+  stdlib_utils
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
+    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?

  rG LLVM Github Monorepo



More information about the libc-commits mailing list