[libc-commits] [PATCH] D118512: [libc] add core printf parsing

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Few highlevel comments for now. Once they are addressed, I will comment on the details.



================
Comment at: libc/src/stdio/printf_parser.h:1
+//===-- Definition of the parser for printf ---------------------*- C++ -*-===//
+//
----------------
Most of this class should move to a `.cpp` file.


================
Comment at: libc/src/stdio/printf_parser.h:33-34
+
+  // TODO(michaelrj): implement a resizable vector class, then convert
+  // token_array and arg_sizes into vectors.
+  size_t token_array_size = 20;
----------------
We should do it now as a prerequisite. Also, instead of a vector, it an "object-store" might be useful in general.


================
Comment at: libc/src/stdio/printf_parser.h:317
+  PrintfParser(const char *__restrict input_str, va_list *in_vlist)
+      : str{input_str}, vlist{in_vlist} {
+    token_array = static_cast<PrintfToken *>(
----------------
While holding onto a `va_list` pointer while on the same stack should be OK, saving it as another object's state can lead to surprising results. Also, it is not clear as to who is responsible for calling `va_start` and `va_end`. Can we perhaps create a RAII class around `va_list` so that we don't have to worry about things like this? Also, it is better to have an API which does not require storing the varargs.


================
Comment at: libc/test/src/stdio/printf_parser_test.cpp:1
+//===-- Unittests for printf_parser ---------------------------------------===//
+//
----------------
Couple of general comments:
1. The tests are testing much less functionality than what is implemented.
2. The `va_list` arg is always empty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118512