Now errno is only set by the terminal entrypoints, and not the internal
implementations. This patch is part of the larger effort to not set
errno in libc internal code: https://github.com/llvm/llvm-project/issues/59278
Details
- Reviewers
sivachandra lntue - Commits
- rG9beb8d1109c0: [libc] move errno out of file internals
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libc/src/__support/File/file.h | ||
---|---|---|
34 | Is this used in the test somewhere? So far all the tests I've seen are directly tested with get_value()? |
libc/src/__support/File/file.h | ||
---|---|---|
43 | For read and write operations, I think is fine with a slight modification. Instead of calling the result type FileResult and making it a template, just define it this way: struct FileIOResult { size_t size; int error; constexpr FileResult(size_t s) : size(s), error(0) {} constexpr FileResult(size_t s, int err) : size(s), error(err) {} constexpr operator size_t() const { return size; } constexpr operator bool() const { return error != 0; } }; The call sites will be like: const auto result = file.write(...) if (!result) errno = result.error; return result; // OK because of operator size_t | |
48 | This is an example of an ErrorOr or StatusOr return value. | |
50 | The close and flush operation functions can return the error value directly. Call sites will detect an error if they see a non-zero return value. |
libc/src/__support/File/file.h | ||
---|---|---|
48 | Actually, we should be use an equivalent of std::expected: https://en.cppreference.com/w/cpp/utility/expected |
move to ErrorOr for the functions that effectively return a boolean. Also build a generic "expected" class.
libc/src/__support/File/file.h | ||
---|---|---|
50 | close and flush call write, meaning that they can theoretically have any error that write can, not just EBADF. |
libc/src/__support/CPP/expected.h | ||
---|---|---|
33 | Does std::expected have constructors of this kind? If no, then we should not add incompatible constructors. | |
36 | Likewise, if has_error is not present, do not include it here. | |
libc/src/__support/File/file.cpp | ||
43 | Do you need a var like buf_result ? Could we just do: auto bytes_written = platform_write(...); if (bytes_written < write_size) { // This should OK because of operator size_t ? ... } | |
52–53 | Same here? | |
libc/src/__support/File/file.h | ||
20 | Is it used with T != size_t anywhere? If not, it need not be a template. | |
50 | EBADF or any error that write returns is still of an integer type. So, call sites can detect error if return value is non-zero. That non-zero value is itself the error. Otherwise, what meaning would the int value in ErrorOr have for flush and close? |
address comments
libc/src/__support/CPP/expected.h | ||
---|---|---|
33 | Yes, this is how the std::expected constructors are defined. | |
libc/src/__support/File/file.cpp | ||
43 | yes. If you look below, we extract the error from buf_result if we're returning an error. | |
52–53 | Same as above, by returning result we preserve the error information. written is only used to determine if the error flag should be set. | |
libc/src/__support/File/file.h | ||
50 | while theoretically we could say that the int result for flush and close actually represents the error instead of the usual return value (which is effectively a boolean for success) I feel like that would be confusing for someone trying to read the codebase. If you're used to a file function named flush that returns 0 on success and -1 on failure (as laid out in the standard), then the internal version of the function actually returning 0 on success and any positive integer on failure could be unexpected. This way it's unambiguous what the error state is. |
libc/src/__support/CPP/expected.h | ||
---|---|---|
33 | Can you point me to a link or section in the standard where its listed? I am looking at cppreference.com and I don't see an unexpected type. I see one which takes std::unexpect_t followed by args for in-place construction of the error value. | |
libc/src/__support/File/file.cpp | ||
43 | You can still do return {0, bytes_written.error}, no? | |
52–53 | Over here, you shouldn't need the written var because you can still compare result < len? I am not advocating for keeping the names as is though. For example, here and above, you can name the var capturing the return value from platform_write as write_result. | |
libc/src/__support/File/file.h | ||
50 | I would actually say using ErrorOr is confusing because one will then start looking for the meaning of the non-error int value. I am not sure the "used to a file function named flush that return 0 ..." is an argument to use - when you return an error ErrorOr value there is no 0 or -1. But, if you actually return int value, then you can still do: if (flush_result != 0) { // Error handling } |
Your optional has a separate storage class:
https://github.com/llvm/llvm-project/blob/ebd3eef0b2b0255e4cf1cd825019ff66d3cd1426/libc/src/__support/CPP/optional.h#L30
Your expected does not.
address comments:
Moved flush and close to returning an int.
Avoided using additional variables where it was unnecessary.
libc/src/__support/CPP/expected.h | ||
---|---|---|
33 | here is std::unexpected: https://en.cppreference.com/w/cpp/error/unexpected |
Minor comments inline, but LGTM.
libc/src/__support/CPP/expected.h | ||
---|---|---|
18 | I think, per https://eel.is/c++draft/expected.unexpected, this should be private? | |
20 | explicit ? Without explicit, the constructor on line 30 below can become ambiguous if T and E are same. | |
33 |
Ah! That stale cppreference page threw me off! That page is actually talking about a removed function std::unexpected and not a type named std::unexpected. But, your interpretation is correct and according to the C++23 draft: https://eel.is/c++draft/expected.unexpected
Thanks! | |
libc/src/__support/File/file.h | ||
20 | I have read this patch on three different days and all those three times the name FileResult has made me a bit uncomfortable. Can we give it a less general name, say FileIOResult or something which makes it clear that this only relates to read/write operations? |
address comments:
Rename FileResult to FileIOResult
Make cpp::unexpected constructor explicit to avoid implicit conversions
Make cpp::unexpected member private and accessed through a getter
libc/src/__support/CPP/expected.h | ||
---|---|---|
22 | The getter should be named error according to this: https://en.cppreference.com/w/cpp/utility/expected/unexpected |
I think, per https://eel.is/c++draft/expected.unexpected, this should be private?