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

[Michael Jones via Phabricator via libc-commits]

michaelrj 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));
   }
----------------
sivachandra wrote:
> `char_converter.h` and `string_converter.h` changes seem to be unrelated to the topic of the patch?
yes. I'll move them to a separate patch.


================
Comment at: libc/src/stdio/printf_core/writer.h:24
+struct WriteBuffer {
+  using OverflowHook = int (*)(WriteBuffer *, void *, cpp::string_view);
+  char *buff;
----------------
sivachandra wrote:
> 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?
That covers almost all the use cases. We still need the `OverflowHook` to be able to modify the buffer to handle the case of `asprintf`, which dynamically allocates a string for its output. In that case when the buffer fills, the pointer may need to be changed and we don't want to reset the current write position.


================
Comment at: libc/src/stdio/printf_core/writer.h:27
+  const size_t buff_len;
+  size_t buff_cur = 0;
 
----------------
sivachandra wrote:
> Why isn't the buffer state kept with the writer? In other words, what is the benefit of this separate buffer type `WriteBuffer`?
It allows us to pass just the `WriteBuffer` to the `OverflowHook`, since the `OverflowHook` needs to be able to modify the buffer based not only on the buffer's state but also the length of the new string being written.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153999