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

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added inline comments.


================
Comment at: libc/src/stdio/fgets.cpp:26
+      reinterpret_cast<__llvm_libc::File *__restrict>(raw_stream);
+  stream->lock();
+
----------------
michaelrj wrote:
> 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.
Ok, never mind then.


================
Comment at: libc/test/src/stdio/fgets_test.cpp:71
+
+    EXPECT_STREQ(buff, output_arr[i]);
+  }
----------------
michaelrj wrote:
> 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.
What I mean is to add:

```
ASSERT_EQ(__llvm_libc::ferror(file), 0);
```

inside of the `for` block above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136785