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

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/src/__support/CPP/expected.h:32
+  constexpr expected(unexpected<E> unexp)
+      : unexp(unexp.value), is_expected(false) {}
+
----------------
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.


================
Comment at: libc/src/__support/File/file.cpp:43
     const size_t write_size = pos;
-    size_t bytes_written = platform_write(this, buf, write_size);
+    auto buf_result = platform_write(this, buf, write_size);
+    size_t bytes_written = buf_result.value;
----------------
sivachandra wrote:
> 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 ?
>   ... 
> }
> ```
yes. If you look below, we extract the error from buf_result if we're returning an error.


================
Comment at: libc/src/__support/File/file.cpp:54
 
-  size_t written = platform_write(this, data, len);
+  auto result = platform_write(this, data, len);
+  size_t written = result.value;
----------------
sivachandra wrote:
> Same here?
Same as above, by returning `result` we preserve the error information. `written` is only used to determine if the error flag should be set.


================
Comment at: libc/src/__support/File/file.h:56
+  using CloseFunc = FileResult<int>(File *);
+  using FlushFunc = FileResult<int>(File *);
 
----------------
sivachandra wrote:
> michaelrj wrote:
> > sivachandra wrote:
> > > 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.
> > close and flush call write, meaning that they can theoretically have any error that write can, not just EBADF.
> `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`?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139576