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

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libc/src/stdio/fgets.cpp:24
+  unsigned char c = '\0';
+  __llvm_libc::File *__restrict stream =
+      reinterpret_cast<__llvm_libc::File *__restrict>(raw_stream);
----------------
Nit: Because `reinterpret_cast` already shows the type, you use `auto`:

```
auto *stream = ...;
```


================
Comment at: libc/src/stdio/fgets.cpp:26
+      reinterpret_cast<__llvm_libc::File *__restrict>(raw_stream);
+  stream->lock();
+
----------------
Use `FileLock` instead:

```
File::FileLock lock(stream);
```

You can use a nested block to reduce the scope of the lock.


================
Comment at: libc/test/src/stdio/fgets_test.cpp:71
+
+    EXPECT_STREQ(buff, output_arr[i]);
+  }
----------------
Add tests for error.


================
Comment at: libc/test/src/stdio/fgets_test.cpp:80
+
+  // Reading more should cause an error.
+  output = __llvm_libc::fgets(buff, 8, file);
----------------
Reading more should still be an EOF and not an error you mean?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136785