[libc-commits] [PATCH] D136785: [libc] add fgets

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/src/stdio/fgets.cpp:26
+      reinterpret_cast<__llvm_libc::File *__restrict>(raw_stream);
+  stream->lock();
+
----------------
sivachandra wrote:
> Use `FileLock` instead:
> 
> ```
> File::FileLock lock(stream);
> ```
> 
> You can use a nested block to reduce the scope of the lock.
I tried using `FileLock` but it's a private part of File.


================
Comment at: libc/test/src/stdio/fgets_test.cpp:71
+
+    EXPECT_STREQ(buff, output_arr[i]);
+  }
----------------
sivachandra wrote:
> Add tests for error.
The only true error state for `fgets` is a read error, which is tested above when I try to read from the write-only file. If there are other error states you see that I don't test then I'll add cases for them, but I don't see any.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136785