[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:26
+  // the null terminator, since that's written separately.
+  StringWriter(char *__restrict init_output, size_t init_max_len = ~size_t(0))
+      : output(init_output), max_len(init_max_len) {}
----------------
sivachandra wrote:
> Nit: Why not `s/init_output/buffer` and call the class member as `buffer_ptr`?
I went with `cur_buffer` since I felt that more accurately described how it was being used.


================
Comment at: libc/src/stdio/printf_core/writer.cpp:20
+  // if the max length is set to 0.
+  if (chars_written + length < max_length)
+    raw_write(output, new_string, length);
----------------
sivachandra wrote:
> The `+` operator can potentially lead to a rollover so the check should be done the other way around? 
> 
> Couple of other points:
> 1. It seems like all this logic redundant? For, the specific writer will/should handle this anyway?
> 1. `max_length` of 0 is also handled by the specific writer anyway?
> 
> May be the `raw_write` should be a "raw" writer anyway. As in, it will just do the writing business and all higher level logic is up to this `Writer` class.
The max length here should have been removed in the previous patch because I moved the max length logic to the string writer class, so yes it was redundant. As for if raw_write should be relatively logic free, I think it makes sense to have the length handled in the string writer, since the max length is only used for strings. Other than that it is relatively raw, only storing a pointer and the number of bytes left to write to it.


================
Comment at: libc/src/stdio/printf_core/writer.cpp:27
+  // regardless of max_length.
+  chars_written += length;
+}
----------------
sivachandra wrote:
> The `+` operation here can also rollover. Is it OK?
`chars_written` is supposed to store the value that will be returned when printf completes. The return value of printf is the number of characters written by that call as represented by an `int`, or a negative number if there was an error. For this purpose, all `chars_written` needs to do is be at least as large as an `int`, and if there would be overflow, return a negative number to represent the error. For this purpose I've changed `chars_written` to an `unsigned long long` so that it will definitely be bigger than an `int`, and the actual printf code can return a negative number if it would overflow that `int`. Finally, `unsigned long long` is guaranteed to have a maximum value of at least 18446744073709551615 (2^64 - 1), and if any call to printf writes more than that many bytes I'm declaring it undefined behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124421