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

[Siva Chandra via Phabricator via libc-commits]

sivachandra 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 {
----------------
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.



================
Comment at: libc/src/__support/str_to_integer.h:155
+  if (result.has_error()) {
+    errno = result.get_error();
+  }
----------------
We should not set `errno` in internal functions, especially in `__support`.


================
Comment at: libc/src/inttypes/strtoimax.cpp:18
                     int base)) {
-  return internal::strtointeger<intmax_t>(str, str_end, base);
+  return internal::strtointeger_errno<intmax_t>(str, str_end, base);
 }
----------------
```
auto result = strtointeger(str, base);
errno = result.conv_status;
*str_end = str + result.parse_length;
return result.conv_val;
```

It might feel like repeated code, but it was already physically repeated in `strtointeger_errno`. Textual repetition is OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133163