[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/string_writer.h:20
+  char *__restrict output;
+  size_t max_len;
+
----------------
Nit: May be call it `available_capacity` as that is how it is being used internally?


================
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) {}
----------------
Nit: Why not `s/init_output/buffer` and call the class member as `buffer_ptr`?


================
Comment at: libc/src/stdio/printf_core/string_writer.h:27
+  StringWriter(char *__restrict init_output, size_t init_max_len = ~size_t(0))
+      : output(init_output), max_len(init_max_len) {}
+  void write(const char *__restrict to_write, size_t len) {
----------------
Nit: Space between line 27 and 28.


================
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);
----------------
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.


================
Comment at: libc/src/stdio/printf_core/writer.cpp:27
+  // regardless of max_length.
+  chars_written += length;
+}
----------------
The `+` operation here can also rollover. Is it OK?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124421