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

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/src/stdio/printf_files/parser.cpp:33
+#ifndef LLVM_LIBC_PRINTF_DISABLE_INDEX_MODE
+    size_t conv_index = parse_index(&cur_pos);
+#define GET_ARG_VAL_SIMPLEST(arg_type, index) get_arg_value<arg_type>(index)
----------------
I feel a little uneasy about only declaring a variable inside one of the branch of `#if` and the always using it (even in the macro form) on all the branches afterward.

Since we are using C++ 17, probably you can use declare the variable `conv_index` with `maybe_unused` attribute (https://en.cppreference.com/w/cpp/language/attributes/maybe_unused) and only assign it to `parse_index(&cur_pos)` under `#if !defined` right after.  In that case, you can move the definition of `GET_ARG_VAL_SIMPLEST` macro to the beginning of the file, outside of the method. 


================
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) {
----------------
Do you only need `const &` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123339