This is an archive of the discontinued LLVM Phabricator instance.

[libc] add result class to strtointeger
ClosedPublic

Authored by michaelrj on Sep 1 2022, 3:22 PM.

Details

Summary

This is a class intended to improve errno handling for internal
functions by allowing functions to return their result and error status
instead of setting errno. This specific class will be used for
strtointeger and (in a followup patch) strtofloat.

Diff Detail

Event Timeline

michaelrj created this revision.Sep 1 2022, 3:22 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 1 2022, 3:22 PM
michaelrj requested review of this revision.Sep 1 2022, 3:22 PM
lntue added inline comments.Sep 1 2022, 3:52 PM
libc/src/__support/error_and.h
24 ↗(On Diff #457423)

I think ValWithErrno, WithErrno or something similar would be more suitable, especially since the constructors have value comes first.

sivachandra added inline comments.Sep 1 2022, 8:28 PM
libc/src/__support/error_and.h
23 ↗(On Diff #457423)

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.

libc/src/__support/str_to_integer.h
153

We should not set errno in internal functions, especially in __support.

libc/src/inttypes/strtoimax.cpp
18–25
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.

michaelrj added inline comments.Sep 2 2022, 1:27 PM
libc/src/__support/error_and.h
23 ↗(On Diff #457423)

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.

24 ↗(On Diff #457423)

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.

libc/src/__support/str_to_integer.h
153

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.

sivachandra added inline comments.Sep 12 2022, 1:52 AM
libc/src/__support/error_and.h
23 ↗(On Diff #457423)

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.
libc/src/__support/str_to_integer.h
153

OK for moving this errno setting template to some other place, say stdlib as you have said.

michaelrj updated this revision to Diff 480636.Dec 6 2022, 2:41 PM
michaelrj marked 3 inline comments as done.

move to specific strtointeger result struct since the new design will have separate return structs for different internal components.

michaelrj retitled this revision from [libc] add ErrorAnd class to strtointeger to [libc] add result class to strtointeger.Dec 6 2022, 2:41 PM
michaelrj edited the summary of this revision. (Show Details)
sivachandra accepted this revision.Dec 7 2022, 3:23 PM
sivachandra added inline comments.
libc/src/__support/str_to_num_result.h
17

You can keep this a struct and avoid the getters. You can choose to keep the has_error method if you want.

20

Nit: str_len can be ambiguous. Name it parsed_length of something more indicative of what it is.

This revision is now accepted and ready to land.Dec 7 2022, 3:23 PM
michaelrj updated this revision to Diff 481358.Dec 8 2022, 10:29 AM
michaelrj marked 5 inline comments as done.

move to struct with directly accessed members instead of getters.

michaelrj updated this revision to Diff 481750.Dec 9 2022, 2:35 PM

fix up cmake names before landing

This revision was landed with ongoing or failed builds.Dec 9 2022, 2:35 PM
This revision was automatically updated to reflect the committed changes.