[libc-commits] [PATCH] D123339: [libc][NFC] implement printf parser

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/src/stdio/printf_files/CMakeLists.txt:1
+
+add_header_library(
----------------
sivachandra wrote:
> It seems to me like directory should be named `printf` without the `_files` suffix. Normally, the `_files` kind of prefixes/suffixes indicate data files and not source files.
I'm worried about name collision between the function and the folder if they're both just named `printf`, so I changed the name to `printf_core` to match the namespace.


================
Comment at: libc/src/stdio/printf_files/parser.cpp:46
+
+      section.min_width = GET_ARG_VAL_SIMPLEST(int, parse_index(&cur_pos));
+    } else if (internal::isdigit(str[cur_pos])) {
----------------
sivachandra wrote:
> I am very likely missing something obvious here: where is `parse_index` defined?
`parse_index` is an index mode function not implemented in this patch. It's in the next patch which contains the index mode code.


================
Comment at: libc/src/stdio/printf_files/parser.h:35
   // TODO: Make this an optional piece.
-  VariableType type_arr[TYPE_ARR_SIZE];
+  uint8_t size_arr[SIZE_ARR_LEN];
 
----------------
sivachandra wrote:
> I think this array is storing the sizes of the var-args. Add a comment saying so. Also, what can be optional and why?
The size array is optional in the sense that it's only needed in index mode, and can be disabled in sequential mode.


================
Comment at: libc/src/stdio/printf_files/parser.h:40
 public:
-  Parser(const char *__restrict str, va_list *vlist);
+  Parser(const char *__restrict new_str, internal::ArgList &args)
+      : str(new_str), args_start(args), args_cur(args) {
----------------
lntue wrote:
> Do you only need `const &` here?
`args` can't be `const` because it's a container for a `va_list` which can't be `const`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123339