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

Siva Chandra via Phabricator via libc-commits libc-commits at lists.llvm.org
Thu Jun 2 01:08:21 PDT 2022

sivachandra added inline comments.

Comment at: libc/src/__support/File/file.cpp:34
+    return written;
+  }
   cpp::ArrayRef<uint8_t> dataref(data, len);
These additional conditionals are making the code even more complicated. Instead, we should do this: rename the current `write_unlocked` method to `write_unlocked_fully_buffered` and make it private. Next, add two new methods like this:

size_t write_unlocked(const void *data, size_t len) {
  if (bufmode == _IOLBF) {
    return write_unlocked_line_buffered(data, len);
  } else if (bufmode == _IOFBF) {
    return write_unlocked_fully_buffered(data, len);
  } else {
    size_t written_size =platform_write(this, data, len);
    // Error handling if platform_write fails.


    // Error handling if platform_flush fails

    return len;

size_t write_unlocked_line_buffered(const void *data, size_t len) {
  // Instead of looking for the first new line char, look for the last new line char.
  char *last = last_newline(data, len);
  if (last == nullptr) {
   // If no new line char is found, just do normal buffering.
    return write_unlocked_fully_buffered(data, len);

  size_t written_size;
  if (pos == 0) {
    // If there is no content in the buffer, write out the new data.
    written_size = platform_write(data, last - data /* size including the new line char */);
    // Perform error handling
  } else {
    // First write out the existing content, and then write out the new data.
    platform_write(buf, pos);
    // Perform error handling
    pos = 0;

    written_size = platform_write(data, last - data);
    // Perform error handling.


  // Perform error handling

  if (written_size == len)
    return len;

  // Remaining data should be buffered.
  written_size = write_unlocked_fully_buffered(data + written_size, len - written_size);

  // Perform error handling

  return len;

Comment at: libc/src/__support/File/file.h:62
+  static constexpr int LINE_BUFFERED = 1;
+  static constexpr int UNBUFFERED = 2;
We shouldn't need this, and may be even not do this. Add the macros `_IOFBF` etc. to `stdio.h` and use them directly.

Comment at: libc/src/__support/File/linux_file.cpp:161
   auto *file = reinterpret_cast<LinuxFile *>(malloc(sizeof(LinuxFile)));
-  LinuxFile::init(
-      file, fd, buffer, File::DEFAULT_BUFFER_SIZE,
-      0, // TODO: Set the correct buffer mode when buffer mode is available.
-      true, modeflags);
+  LinuxFile::init(file, fd, buffer, File::DEFAULT_BUFFER_SIZE, File::UNBUFFERED,
+                  true, modeflags);
Shouldn't the default buffering mode be `_IOFBF`?

Comment at: libc/src/__support/File/linux_file.cpp:173-174
 constexpr size_t STDERR_BUFFER_SIZE = 1024;
 char stderr_buffer[STDERR_BUFFER_SIZE];
+static LinuxFile StdErr(2, stderr_buffer, STDERR_BUFFER_SIZE, File::UNBUFFERED,
We don't need these anymore - just make the size arg `0` and buffer arg `nullptr` in the constructor call below.

Comment at: libc/test/src/__support/File/file_test.cpp:110
                             bool owned, const char *mode) {
-  StringFile *f = reinterpret_cast<StringFile *>(malloc(sizeof(StringFile)));
+  StringFile *f = reinterpret_cast<StringFile *>(calloc(1, sizeof(StringFile)));
   f->init(buffer, buflen, bufmode, owned, __llvm_libc::File::mode_flags(mode));
Why did you change this to `calloc`?

  rG LLVM Github Monorepo



More information about the libc-commits mailing list