[libc-commits] [PATCH] D124421: [libc] add printf writer

Siva Chandra via Phabricator via libc-commits libc-commits at lists.llvm.org
Wed Apr 27 14:47:28 PDT 2022

sivachandra added inline comments.

Comment at: libc/src/stdio/printf_core/CMakeLists.txt:28
+  libc.src.string.memory_utils.memcpy_implementation

Comment at: libc/src/stdio/printf_core/string_writer.h:24
+// after the call to printf_main, point to the end of the string, and it should
+// be set to '\0' by the higher level call.
+void write_to_string(void *raw_pointer, const char *__restrict to_write,
It seems to me like you want a stateful writer. So, why not just make it a class?

Comment at: libc/src/stdio/printf_core/writer.h:31
+  // buffer it's writing to. This is because it doesn't handle the null
+  // terminator.
   size_t max_length;
Can you explain the need for `max_length`? It seems to me like it is something a specific writer, say `StringWriter` should care about, but not a generic writer?

At this point, I would actually ask the need for a generic `Writer` class. It seems to me like all you need is a writer class which implements a method with a specific signature. For example, you want `StringWriter` like this:

class StringWriter {
  ... // State as suggested in another comment

  // This method attempts to write |len| bytes from |data| and return the actual
  // number of bytes written.
  size_t write(const void *data, size_t len);

FWIW, the `File` class already implements the `write` protocol. So, based on the printf function, `sprintf` or `fprintf`, one either uses the `StringWriter` or `File`.

Comment at: libc/src/stdio/printf_core/writer.h:33
   size_t max_length;
-  size_t chars_written;
+  size_t chars_written = 0;
Is this required only for testing or do you need it for something else also?

  rG LLVM Github Monorepo



More information about the libc-commits mailing list