[libc-commits] [PATCH] D131994: [libc] move printf to use StringViews

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/stdio/printf_core/char_converter.h:35
+
+  RET_IF_RESULT_NEGATIVE(writer->write(cpp::StringView(&c, string_len)));
+
----------------
Here and elsewhere, instead of calling the `StringView` constructor explicitly, can you do `{&c, string_len}`?


================
Comment at: libc/src/stdio/printf_core/file_writer.h:37
 // function.
-int write_to_file(void *raw_pointer, const char *__restrict to_write,
-                  size_t len);
+int write_str_to_file(void *raw_pointer, cpp::StringView new_string);
+
----------------
While we are at it, we should make these static methods of `FileWriter` and simplify the names to `write_str` and `write_chars`.


================
Comment at: libc/src/stdio/printf_core/string_writer.h:41
+// function on new_string.
+int write_str_to_string(void *raw_pointer, cpp::StringView new_string);
+
----------------
Make these static methods of `StringWriter`.


================
Comment at: libc/src/stdio/printf_core/writer.h:49
+  // chars_write.
+  int write_chars(char new_char, size_t len);
 
----------------
Something we should have done earlier is to name all of these methods as just `write`. So, I think we should have:

```
int write(cpp::StringView);
int write(char c, size_t count);
int write(char c);
```

I am suggesting a separate `write(char)` to avoid calling, for example, `memcpy` to write a single char to a string buffer. Likewise, the `FileWriter` can implement a char writer by calling `putc_unlocked` when it is available. More importantly though, you will avoid the ugliness of `writer->write(StringView(&some_char, 1))` at many places in this change.


================
Comment at: libc/test/src/stdio/printf_core/parser_test.cpp:57
+
+  expected.raw_string = StringView(str, 4);
 
----------------
Can you do:

```
expected.raw_string = {str, 4};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131994