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

Siva Chandra via Phabricator via libc-commits libc-commits at lists.llvm.org
Mon Feb 14 07:56:51 PST 2022

sivachandra marked 2 inline comments as done.
sivachandra added inline comments.

Comment at: libc/include/CMakeLists.txt:126-127
+    .llvm-libc-macros.stdio_macros
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
+  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.

  rG LLVM Github Monorepo



More information about the libc-commits mailing list