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

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
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,
----------------
sivachandra wrote:
> It seems to me like you want a stateful writer. So, why not just make it a class?
it felt like overkill for tracking only one variable, but if I'm moving `max_len` in there as well, then it makes more sense.


================
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;
----------------
sivachandra wrote:
> 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`.
File doesn't support all of the functions I need, specifically it doesn't have `chars_written`, which represents the number of characters written by this specific call to printf and is used both in the `%n` conversion and as the return value for all printf functions. 

I have moved `max_length` into the `StringWriter` class now though.


================
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;
----------------
sivachandra wrote:
> michaelrj wrote:
> > sivachandra wrote:
> > > 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`.
> > File doesn't support all of the functions I need, specifically it doesn't have `chars_written`, which represents the number of characters written by this specific call to printf and is used both in the `%n` conversion and as the return value for all printf functions. 
> > 
> > I have moved `max_length` into the `StringWriter` class now though.
> Actually, I am beginning think there is benefit with the generic `Writer` indirection. Consider the `File` example again - file writer might want to explicitly lock and use unlocked file methods to write to the file. Having a generic writer allows us to hide such details in the file writer.
I agree, and theoretically on platforms that don't care as much about code size we could have two string writer versions, one with a max length and one without, since that would improve performance.


================
Comment at: libc/src/stdio/printf_core/writer.h:33
   size_t max_length;
-  size_t chars_written;
+  size_t chars_written = 0;
 
----------------
sivachandra wrote:
> Is this required only for testing or do you need it for something else also?
`chars_written` is the return value of printf, as well as the value to be written for `%n`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124421