[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/writer.h:24
+struct WriteBuffer {
+  using OverflowHook = int (*)(WriteBuffer *, void *, cpp::string_view);
+  char *buff;
----------------
michaelrj wrote:
> 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.
`asprintf` should be implemented with the separation of abstractions intact. So:

1. The `printf` writer works with a statically allocated (stack) buffer similar to `fprintf`.
2. The `StreamWriter` is responsible for maintaining the expanding output buffer. The `void *` argument to of `StreamWriter` points to the expandable buffer (likely encapsulated with additional state).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153999