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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

In D107792#2937562 <https://reviews.llvm.org/D107792#2937562>, @michaelrj wrote:

> I was planning on using `internal::strtoll` to implement `atoi` and friends, but I'm not sure what the additional errno argument you mentioned would do. The existing implementation of `atoi` gives errors in the same manner as `strtoll`, e.g. setting `errno` to `ERANGE` when passed a string that decodes to a number greater than 2^63.

If I am reading the C standard and the POSIX standard correctly, `atoi` and friends do not modify `errno`. So, what I was trying to point out is that you might want to pass an additional `int &` argument for errno which you will use in `strtoll` and friends but not in `atoi` and friends.



================
Comment at: libc/src/__support/stdlib_utils.h:12
+
+#include "include/errno.h"
+#include "src/__support/ctype_utils.h"
----------------
michaelrj wrote:
> 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.
Ah, as a follow up, we should revisit this and may be move some of the functions outside the conditional.


================
Comment at: libc/src/__support/stdlib_utils.h:78
+
+  unsigned long long const CUR_LLONG_MAX =
+      (result_sign == '+' ? LLONG_MAX
----------------
michaelrj wrote:
> 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.
Call it `ABS_MAX` may be?


================
Comment at: libc/src/__support/stdlib_utils.h:103
+  if (str_end != nullptr)
+    *str_end = (char *)(src);
+  if (result_sign == '+')
----------------
michaelrj wrote:
> 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?
`*str_end = const_cast<char *>(src);` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107792