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

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/src/__support/stdlib_utils.h:12
+
+#include "include/errno.h"
+#include "src/__support/ctype_utils.h"
----------------
sivachandra wrote:
> 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.
I have switched it, but all of the stdlib functions are inside of an `if(LLVM_LIBC_FuLL_BUILD)` in `entrypoints.txt`, so if full build is off then it won't even try to build this.


================
Comment at: libc/src/__support/stdlib_utils.h:78
+
+  unsigned long long const CUR_LLONG_MAX =
+      (result_sign == '+' ? LLONG_MAX
----------------
sivachandra wrote:
> 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`.
In this case it's to distinguish it from LLONG_MAX, since this is the current maximum value we can store (either `LLONG_MAX` or `-LLONG_MIN`). I'm not sure what other prefix to use to indicate that this is a local variable, but a const one.


================
Comment at: libc/src/__support/stdlib_utils.h:103
+  if (str_end != nullptr)
+    *str_end = (char *)(src);
+  if (result_sign == '+')
----------------
This line gives me compiler warnings due to casting from a `const char*` to a `char*` but the specification says that `str_end` is not const. Is there a better way to do this or alternately a way to suppress the compiler warnings?


================
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);
----------------
sivachandra wrote:
> Why not `LLONG_MAX`?
I was trying to avoid including `<limits.h>`, but on reflection having the variables match across the files is more important than that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107792