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

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added a comment.
This revision is now accepted and ready to land.

LGTM. I have left a bunch of nits. Also, we might want to adjust the structuring a little after the converter is implemented.



================
Comment at: libc/src/stdio/printf_core/core_structs.h:18
 
 enum class LengthModifier { hh, h, l, ll, j, z, t, L, none };
 
----------------
Add a comment to explain the style exception here.


================
Comment at: libc/src/stdio/printf_core/core_structs.h:27
+  LeadingZeroes = 0x10, // 0
+  // group_decimals = 0x20, // '
+  // locale_digits = 0x40,  // I
----------------
Nit: Either remove or add a comment to explain why they are commented out.
Also, the enum values are constants, so we should list them in `THIS_STYLE`?


================
Comment at: libc/src/stdio/printf_core/core_structs.h:30
+
+  // Default:
+  Clear = 0,
----------------
Nit: When we have bit flags, 0 is the implicit no flag value. You don't need a symbolic name.


================
Comment at: libc/src/stdio/printf_core/parser.cpp:20
+
+#define LLVM_LIBC_PRINTF_DISABLE_INDEX_MODE 1 // This will be a compile flag.
+
----------------
If this patch is only about the sequential mode, then why do we need to worry about the index mode at all currently? At the least, it is confusing because a reader will go looking for unimplemented pieces like `parse_index`.


================
Comment at: libc/src/stdio/printf_core/parser.h:31-32
+  static constexpr size_t SIZE_ARR_LEN = 32;
+  static constexpr uint8_t FLOAT_MASK = 0x80;
+  static constexpr uint8_t SIZE_MASK = 0x7f;
+  // size_arr stores the sizes of the variables in the ArgList. This is used in
----------------
Seems to me like `FLOAT_MASK` and `SIZE_MASK` are unused. So, remove them until they are actually used somewhere?


================
Comment at: libc/src/stdio/printf_core/parser.h:59
+  // arithmetically or'd together. local_pos will be moved past any flags found.
+  FormatFlags parse_flags(size_t *local_pos);
+
----------------
Why are these functions taking an arg? Seems to me like they update `cur_pos` anyway?


================
Comment at: libc/test/src/stdio/printf_core/parser_test.cpp:19
+public:
+  void assert_eq_fs(__llvm_libc::printf_core::FormatSection expected,
+                    __llvm_libc::printf_core::FormatSection actual) {
----------------
You should ideally implement a matcher but you can do it separately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123339