[libc-commits] [PATCH] D153396: WIP: [libc] Add fdopen for linux targets

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

This overall OK. But, I would like to spend some more time thinking about the new abstractions.



================
Comment at: libc/src/__support/File/linux/file.cpp:105
 
+ErrorOr<File *> allocatefile(int fd, File::ModeFlags modeflags) {
+  if (fd < 0)
----------------
This function can move into the anonymous namespace?


================
Comment at: libc/src/__support/File/linux/file.cpp:134
+
+  if (modeflags & ModeFlags(File::OpenMode::APPEND)) {
+    auto cur_flags = __llvm_libc::syscall_impl<int>(SYS_fcntl, fd, F_GETFL);
----------------
Can you add comments explaining why `APPEND` is handled as a special case?


================
Comment at: libc/test/src/stdio/fdopen_test.cpp:29
+  ASSERT_THAT(__llvm_libc::fclose(file), Succeeds(0));
+}
----------------
You should include few negative tests, say calling `fdopen` with a bad `fd`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153396