[libc-commits] [PATCH] D119458: [libc] Add a platform independent buffered file IO data structure.

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/include/CMakeLists.txt:126-127
   DEPENDS
     .llvm_libc_common_h
+    .llvm-libc-macros.stdio_macros
     .llvm-libc-types.FILE
----------------
I just notice that we have both *-* and *_* here.  We should update to use just 1 style at some point.


================
Comment at: libc/src/__support/File/file.cpp:170
+  }
+  free(this);
+  return 0;
----------------
Shouldn't you only need:

```
if (own_buf) free(this->buf);
```


================
Comment at: libc/src/__support/File/file.h:145
+  // Sets the internal buffer to |buffer| with buffering mode |mode|.
+  // |size| is the size of |buffer|. This new |buffer| is not owned by the
+  void set_buffer(char *buffer, size_t size, bool owned);
----------------
comment ended early.


================
Comment at: libc/test/src/__support/File/file_test.cpp:20
+
+class StringFile : public __llvm_libc::File {
+  static constexpr size_t SIZE = 512;
----------------
I feel like this class will be useful beyond this test and could be factored to a standalone util.  Feel free to ignore this suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119458