[libc-commits] [PATCH] D123424: [libc][NFC] add index mode to printf parser

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

I have not yet gone through the tests.



================
Comment at: libc/src/stdio/printf_core/parser.cpp:380
+  while (args_index < index) {
+    Parser::TypeRep cur_size = get_arg_size<void>();
+    if (args_index <= SIZE_ARR_LEN)
----------------
Nit: `s/cur_size/cur_type_desc` ?


================
Comment at: libc/src/stdio/printf_core/parser.h:34
 
+  enum SuperType : uint8_t { Integer, Float, Pointer };
+
----------------
Nit: Call it `PrimaryType` may be? The word super can be misleading because of other concepts like super classes.


================
Comment at: libc/src/stdio/printf_core/parser.h:38
+  // relatively compact manner.
+  struct TypeRep {
+    uint8_t size;
----------------
Nit: Normally, things like this are named `TypeDesc` as a short for "type descriptor".


================
Comment at: libc/src/stdio/printf_core/parser.h:52
+  // integer values in va_args.
+  TypeRep size_arr[SIZE_ARR_LEN];
+
----------------
Nit: This is not an array of sizes anymore.


================
Comment at: libc/src/stdio/printf_core/parser.h:109
+  // adding the float mask for floating point types.
+  template <class T> static constexpr TypeRep inline get_arg_size() {
+    TypeRep val;
----------------
Looks like you can use a constexpr variable template instead of these functions?


================
Comment at: libc/src/stdio/printf_core/parser.h:140
+
+  void inline set_size_arr(size_t index, TypeRep value) {
+    if (index != 0 && index <= SIZE_ARR_LEN)
----------------
Nit: `s/set_size_arr/set_type_desc/` ?


================
Comment at: libc/src/stdio/printf_core/parser.h:167
+  // variable for index. If no appropriate format specifier is found, it
+  // returns 32.
+  TypeRep size_of_index(size_t index);
----------------
Update the comment appropriately?


================
Comment at: libc/src/stdio/printf_core/parser.h:168
+  // returns 32.
+  TypeRep size_of_index(size_t index);
+
----------------
Nit: `s/size_of_index/get_type_desc/` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123424