[libc-commits] [PATCH] D139576: [libc] move errno out of file internals

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added a comment.
This revision is now accepted and ready to land.

Minor comments inline, but LGTM.



================
Comment at: libc/src/__support/CPP/expected.h:32
+  constexpr expected(unexpected<E> unexp)
+      : unexp(unexp.value), is_expected(false) {}
+
----------------
michaelrj wrote:
> sivachandra wrote:
> > michaelrj wrote:
> > > sivachandra wrote:
> > > > Does `std::expected` have constructors of this kind? If no, then we should not add incompatible constructors.
> > > Yes, this is how the `std::expected` constructors are defined.
> > 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.
> here is std::unexpected: https://en.cppreference.com/w/cpp/error/unexpected
> It's used in constructors 7 and 8 in the list on this page: https://en.cppreference.com/w/cpp/utility/expected/expected
> here is std::unexpected: https://en.cppreference.com/w/cpp/error/unexpected

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

> It's used in constructors 7 and 8 in the list on this page: https://en.cppreference.com/w/cpp/utility/expected/expected

Thanks!


================
Comment at: libc/src/__support/CPP/expected.h:17
+template <class T> struct unexpected {
+  T value;
+
----------------
I think, per https://eel.is/c++draft/expected.unexpected, this should be private?


================
Comment at: libc/src/__support/CPP/expected.h:19
+
+  constexpr unexpected(T value) : value(value) {}
+};
----------------
`explicit` ? Without explicit, the constructor on line 30 below can become ambiguous if `T` and `E` are same.


================
Comment at: libc/src/__support/File/file.h:20
 
+struct FileResult {
+  size_t value;
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139576