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

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/src/__support/File/file.cpp:47
+  // be buffered if there's space.
+  size_t flush_point = bufspace;
+
----------------
sivachandra wrote:
> While the concept of `flush_point` seems promising, I think there is a mix up of a `xfer_point` and a `flush_point` in the code below. You want to transfer and clear the libc buffer when it is full. But, under `_IOLBF`, you want to actually perform the flush operation. May be I am missing it, but I don't see any call to `platform_flush` after this line.
I've added a call to flush at the end.


================
Comment at: libc/src/__support/File/file.cpp:147
 
-  size_t transferred =
-      platform_write(this, dataref.data() + write_size, remaining);
-  if (transferred < remaining) {
-    err = true;
-    return write_size + transferred;
+  size_t remaining = len - flush_point;
+
----------------
sivachandra wrote:
> Should this be:
> 
> ```
> size_t remaining = len - write_size;
> ```
in this case `remaining` represents the number of characters left after the flush point. This is useful, since we want to know if the characters after the flush point will fit into the buffer, and if they will we want to write everything to output directly.


================
Comment at: libc/src/__support/File/file.cpp:151
+  // write some characters from data directly.
+  if (flush_point > bufspace) {
+    // if the remaining characters after the flush point won't fit in the
----------------
sivachandra wrote:
> Should `remaining` be updated inside this `if` block? Couple of points:
> 
> 1. It seems like the condition is true only for a specific case of `_IOLBF`.
> 1. This condition can be true even if there is no newline char in the input?
no, since it's only tracking the number of characters after the flush point, and from that the number of characters written should be logical after each step.

This isn't only true in a special case of line buffered mode, but it is related to that. This handles the case where the flush point is after the end of the buffer, as well as the cases where the remainder after the flush point doesn't fit into the buffer. The flush point can only be after the end of the buffer if there's a newline, because otherwise the flush point is equal to the end of the buffer.

It should be able to be true if there's no newline in the input, that's a good catch. I've added an extra condition.


================
Comment at: libc/src/__support/File/file.cpp:171
+    pos = remaining; // to reach this point, the buffer must have been flushed.
+  }
+
----------------
sivachandra wrote:
> Is an `else` missing here?
no, but there used to be one here. If this were only fully buffered then writing `remaining` to the buffer and writing from `data` to the output directly would never happen together, but in line buffered mode in the case where the newline is after the end of the buffer, but the characters after the newline fit into the buffer, we might need to do both.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126829