[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) {}
+
----------------
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.


================
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;
----------------
michaelrj wrote:
> 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.
You can still do `return {0, bytes_written.error}`, no?


================
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;
----------------
michaelrj wrote:
> 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.
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`.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139576