[libcxx-commits] [PATCH] D158930: [libc++][print] Fixes error reporting.

[Elliott Hughes via Phabricator via libcxx-commits]

enh added inline comments.


================
Comment at: libcxx/include/print:218-219
 
+  // On POSIX errno is set by fwrite. This is not required by the C standard.
+  errno         = 0;
   size_t __size = fwrite(__str.data(), 1, __str.size(), __stream);
----------------
rprichard wrote:
> ldionne wrote:
> > I am a bit concerned with the direct usage of `errno` here. What about thread safety?
> > 
> > IMO the previous approach with `ferror` made more sense, we just shouldn't be using the return value of `ferror` as more than a zero/non-zero indicator. WDYT?
> POSIX defines errno as thread-local, and a non-thread-local errno would be mostly unusable:
>  - https://stackoverflow.com/a/1694170
>  - https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html#tag_22_02_03
> 
> Maybe it'd make sense to save/restore errno in case the caller expects its value to be unaffected by a C++ API. Not sure. (e.g. Bionic has a ErrnoRestorer class that it uses to avoid leaking errno modifications to callers.)
> 
> I don't really have an opinion right now on feof and ferror usage, except that the Bionic-vs-glibc difference makes me wonder if ferror would give inconsistent results (https://reviews.llvm.org/D150044#4597254).
> 
/me wonders whether this should all just be deleted anyway until/unless someone comes along with an actual motivating case for trying to be more specific?

this seems fine?
```
  if (__size < __str.size()) {
    std::__throw_system_error(EIO, "failed to write formatted output");
  }
```
POSIX explicitly states that a fwrite() that returns less than you asked it to write should set the error indicator (so by that, ferror() must always be non-zero, so there's no reason to explicitly check it).

but, yeah, if you do want to be fancy, the "save/restore errno" code sgtm:
```
  int saved_errno = errno;
  errno = 0;
  size_t __size = fwrite(__str.data(), 1, __str.size(), __stream);
  if (__size < __str.size()) {
    std::__throw_system_error(errno, "failed to write formatted output");
  }
  errno = saved_errno;
```
(it looks like std::system_error already does the right thing should errno still be zero because the fwrite() failure was not actually an error from the OS --- because the multiplication overflowed or whatever [not that that can happen multiplying by 1 like here, but ykwim!].)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158930