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

Siva Chandra via Phabricator via libc-commits libc-commits at lists.llvm.org
Fri Apr 8 11:47:19 PDT 2022

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
+  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.

  rG LLVM Github Monorepo



More information about the libc-commits mailing list