[libc-commits] [PATCH] D126829: [libc] add buffering to FILE writes

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Good to go after the comments addressed.



================
Comment at: libc/src/__support/File/file.cpp:141
+
+  // If the primary piece fits in the buffer, copy the it into the buffer.
+  if (primary.size() <= bufspace) {
----------------
We will keep the implementation simple for now like this:

```
fbf(primary.data(), primary.size());
// Error check
flush_unlocked(); // A call to flush and not to platform_flush()
fbf(remaining.data(), remaining.size());
// Error check
```

It should incur the same number of `platform_*` calls but might lead to a few additional copies. Iff required, we can optimize FBF to eliminate the unecessary copies. Infact, what we really want is to replace the first FBF operation by an NBF operation. We can do all that iff required.

I don't think we have `flush_unlocked`, so you will have to add it.


================
Comment at: libc/test/src/__support/File/file_test.cpp:155
+  char file_buffer[FILE_BUFFER_SIZE];
+  StringFile *f =
+      new_string_file(file_buffer, FILE_BUFFER_SIZE, _IOLBF, false, "w");
----------------
It will be cool to actually create an FBF file also and show the difference in pos comparisons (like those on line 170 and 174).


================
Comment at: libc/test/src/__support/File/file_test.cpp:397
+  char file_buffer[FILE_BUFFER_SIZE];
+  StringFile *f = new_string_file(file_buffer, FILE_BUFFER_SIZE, 0, false, "w");
+
----------------
`_IOFBF`?


================
Comment at: libc/test/src/__support/File/file_test.cpp:411
+  constexpr size_t WRITE_SIZE = sizeof(WRITE_DATA);
+  StringFile *f = new_string_file(nullptr, 0, 0, true, "w");
+
----------------
Use some buffering mode. Or, test for all modes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126829