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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/__support/CPP/expected.h:32
+  constexpr expected(unexpected<E> unexp)
+      : unexp(unexp.value), is_expected(false) {}
+
----------------
Does `std::expected` have constructors of this kind? If no, then we should not add incompatible constructors.


================
Comment at: libc/src/__support/CPP/expected.h:35
+  constexpr bool has_value() { return is_expected; }
+  constexpr bool has_error() { return !is_expected; }
+
----------------
Likewise, if `has_error` is not present, do not include it here.


================
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;
----------------
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 ?
  ... 
}
```


================
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;
----------------
Same here?


================
Comment at: libc/src/__support/File/file.h:20
 
+template <typename T> struct FileResult {
+  T value;
----------------
Is it used with `T` != `size_t` anywhere? If not, it need not be a template.


================
Comment at: libc/src/__support/File/file.h:56
+  using CloseFunc = FileResult<int>(File *);
+  using FlushFunc = FileResult<int>(File *);
 
----------------
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`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139576