[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 {
----------------
michaelrj wrote:
> 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.
A struct is not any different operationally from `ErrorAnd` and would be more `strtointeger` specific. The adapter code is all confined to just one place (like the template helper you have below.) But, the point you make about `string_view` is valid not just because of `strlen`, but even correctness wise:
1. `string_view` has a constructor which can initialize from a raw pointer: https://github.com/llvm/llvm-project/blob/main/libc/src/__support/CPP/string_view.h#L67
2. But, there is not guarantee that the raw pointer is null-terminated. So, we cannot use `string_view` as you have concluded.


================
Comment at: libc/src/__support/str_to_integer.h:155
+  if (result.has_error()) {
+    errno = result.get_error();
+  }
----------------
michaelrj wrote:
> 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.
OK for moving this errno setting template to some other place, say `stdlib` as you have said.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133163