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

[Siva Chandra via Phabricator via libc-commits]

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:

```
public:
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.

    platform_flush(this);

    // Error handling if platform_flush fails

    return len;
  }
}

private:
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.
  }


  platform_flush();

  // 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`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126829