[libc-commits] [PATCH] D153999: [libc] Move printf writer to new design

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/stdio/printf_core/char_converter.h:32
       (to_conv.flags & FormatFlags::LEFT_JUSTIFIED) == 0) {
-    RET_IF_RESULT_NEGATIVE(writer->write(' ', to_conv.min_width - string_len));
+    RET_IF_RESULT_NEGATIVE(writer->write(' ', padding_spaces));
   }
----------------
`char_converter.h` and `string_converter.h` changes seem to be unrelated to the topic of the patch?


================
Comment at: libc/src/stdio/printf_core/vfprintf_internal.h:33
+using __llvm_libc::funlockfile;
+using __llvm_libc::fwrite_unlocked;
+#else  // defined(LIBC_COPT_PRINTF_USE_SYSTEM_FILE)
----------------
This is potentially leaking entrypoints. Is simplicity of `flush_and_write` and `vfprintf_internal` the only reason? Can you instead do this:

```
namespace internal {

#ifdef LIBC_COPT_PRINTF_USE_SYSTEM_FILE
LIBC_INLINE int ferror_unlocked(FILE *f) {
  return ::ferror_unlocked(f);
}
...
#else
LIBC_INLINE int ferror_unlocked(FILE *f) {
  return reinterpret_cast<__llvm_libc::File *>(f)->ferror_unlocked(f);
}
...
#endif

} // namespace internal
```


================
Comment at: libc/src/stdio/printf_core/writer.h:24
+struct WriteBuffer {
+  using OverflowHook = int (*)(WriteBuffer *, void *, cpp::string_view);
+  char *buff;
----------------
Looking at the implementation of the hooks above, it seems to me that the expectation is for the hook to adjust the buffer state. I would rather have a single internal place to manage all internal state. So, I suggest the following changes:

1. Call this hook `StreamWriter` instead of `OverflowHook`. This hook should just do the job of writing to the underlying stream and return the errors it might run in to.
2. Let the `Writer` class alone manage the internal state.
3. The `StreamWriter` hook should only be called when the writer object's buffer fills up (or at the end of course).
4. If `StreamWriter` is `nullptr`, then the writer should drop write data on the floor once the buffer fills up.

Does it cover all use cases?


================
Comment at: libc/src/stdio/printf_core/writer.h:27
+  const size_t buff_len;
+  size_t buff_cur = 0;
 
----------------
Why isn't the buffer state kept with the writer? In other words, what is the benefit of this separate buffer type `WriteBuffer`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153999