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

[Siva Chandra via Phabricator via libc-commits]

sivachandra marked 2 inline comments as done.
sivachandra 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
----------------
lntue wrote:
> I just notice that we have both *-* and *_* here.  We should update to use just 1 style at some point.
Agreed. I will make another pass to fix this.


================
Comment at: libc/src/__support/File/file.cpp:170
+  }
+  free(this);
+  return 0;
----------------
lntue wrote:
> Shouldn't you only need:
> 
> ```
> if (own_buf) free(this->buf);
> ```
The memory for the File data structure itself will be created by a `malloc` call so it has to be free-d at closing time. But, thanks for pointing out about the buffer. I have now added what you have suggested.


================
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);
----------------
lntue wrote:
> comment ended early.
This is not it the correct place anymore, but I think I have addressed this comment.


================
Comment at: libc/test/src/__support/File/CMakeLists.txt:1
+add_libc_unittest(
+  file_test
----------------
michaelrj wrote:
> this needs to be conditioned on malloc being available, which currently means `if(LLVM_LIBC_INCLUDE_SCUDO OR NOT LLVM_LIBC_FULL_BUILD)`. I will go look into making a single variable that can encompass that in a cleanup patch.
Not required anymore after your fix from last week.


================
Comment at: libc/test/src/__support/File/file_test.cpp:20
+
+class StringFile : public __llvm_libc::File {
+  static constexpr size_t SIZE = 512;
----------------
lntue wrote:
> 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.
SGTM. I will move it to a common place as we start reusing it in other tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119458