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

Siva Chandra via Phabricator via libc-commits libc-commits at lists.llvm.org
Tue Feb 1 15:51:30 PST 2022

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.

  rG LLVM Github Monorepo



More information about the libc-commits mailing list