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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/stdio/printf_core/CMakeLists.txt:28
+  DEPENDS
+  libc.src.string.memory_utils.memcpy_implementation
+)
----------------
Indent.


================
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 {
private:
  ... // State as suggested in another comment
public:

  // 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124421