[libc-commits] [PATCH] D133163: [libc] add ErrorAnd class to strtointeger

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/src/__support/error_and.h:23
+// errno, which may help performance. Additionally, optimization passes may be
+// able to remove the error code if the result is discarded.
+template <typename T> class ErrorAnd {
----------------
sivachandra wrote:
> Going by the use case in `strtointeger`, it doesn't feel like this is the appropriate data structure. The use cases always return a value and some times need an error status also. So, I suggest that strointeger only return a data type like:
> 
> ```
> <typename T>
> struct StrToIntegerResult {
>   // Errors during conversion are represented with error values from errno.h.
>   int conv_status = 0;
>   // Length of the input string parsed.
>   size_t parse_length = 0;
>   // Conversion result.
>   T result = 0;
> };
> ```
> 
> The `strtointeger` header is modified as:
> 
> ```
> // As this is a template, we don't need to mark this with `static inline`.
> template <typename T>
> StrToIntegerResult<T> strtointeger(cpp::string_view src, int base) {
>  ...
> }
> ```
> 
> See below for example how this is to be used at call sites.
> 
While we could move to a full struct containing the output, I like the idea of sticking closer to the original prototype of the function. It means less adaptor code and avoids defining a struct for this one function. Additionally, a `stringview` doesn't work as an input for this function, since it's not provided with a string length. While we could effectively call strlen on the string, that isn't helpful as one of the outputs of this function is already the length of the integer parsed which is less than or equal to the total length of the string.


================
Comment at: libc/src/__support/error_and.h:24
+// able to remove the error code if the result is discarded.
+template <typename T> class ErrorAnd {
+  T return_val;
----------------
lntue wrote:
> I think `ValWithErrno`, `WithErrno` or something similar would be more suitable, especially since the constructors have `value` comes first.
I like the name `ErrorAnd` because when it's templated you get a name like `ErrorAnd<int>`. Additionally, for functions that don't return a value on error I've briefly discussed the idea of an `ErrorOr` class, and I don't know if there's as good a parallel for `WithErrno`. Ultimately it's just a name though, and I don't have a particularly strong opinion.


================
Comment at: libc/src/__support/str_to_integer.h:155
+  if (result.has_error()) {
+    errno = result.get_error();
+  }
----------------
sivachandra wrote:
> We should not set `errno` in internal functions, especially in `__support`.
I feel that it's more important to not set errno //unexpectedly// in `__support`, as opposed to at all. While it's true we could just repeat the code in `strtointeger_errno` in every entrypoint to avoid having errno in here that feels like unnecessary repetition. The code isn't just similar, it's literally the same. It's also used in 9 different entrypoints in 2 different folders, which leads to a significant chance for mistakes during refactors. If you really don't want errno in `__support` then we can move this errno wrapper into a shared header in `stdlib`, but I don't think having it identical in every entrypoint is good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133163