[libc-commits] [PATCH] D122773: [libc][NFC] add outline of printf

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/src/stdio/printf_files/writer.h:29
+
+  size_t max_length;
+  size_t chars_written;
----------------
sivachandra wrote:
> What is `max_length`?
for `snprintf` there is a limit on the maximum number of characters that can be written. This represents that maximum length. In other cases it will be set to a default (probably size_t maximum).


================
Comment at: libc/src/stdio/printf_files/writer.h:38
+  // written. It always increments chars_written by length.
+  void write(const char *new_string, size_t length);
+
----------------
sivachandra wrote:
> These functions should be implemented inline to avoid multiple indirections?
Could you clarify what you mean by implementing this inline? This function won't just be calling the raw_write function. If length + chars_written > max_length then the length passed to raw_write will be max_length - chars_written, so that this doesn't overrun the buffer.


================
Comment at: libc/src/stdio/printf_files/writer.h:20
+
+class Writer {
+  void *output;
----------------
sivachandra wrote:
> michaelrj wrote:
> > sivachandra wrote:
> > > Is this class envisioned to be a `final` class? If yes, then we should have documentation of the expectations. For example, what is `output`, `raw_write` etc. If not also, we should add documentation about how it is to be specialized.
> > I was under the impression that virtual functions were disallowed in libc, so all classes were implicitly final.
> > 
> > I've added comments with more explanation on how this specialization works.
> Couple of things:
> 1. A class can be final without a virtual function.
> 2. Going by your comments, this is class is a wrapper over the actual write streams. Which makes me feel it is a final class (as in, the intention is not to specialize it.) You don't need to mark it final though.
I've marked it as final for clarity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122773