[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_core/parser.cpp:20
+
+#define LLVM_LIBC_PRINTF_DISABLE_INDEX_MODE 1 // This will be a compile flag.
+
----------------
sivachandra wrote:
> 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`.
This is here because I wrote the parser with index mode in mind, and then scaled it back for this patch. I'll remove references to index mode for now for clarity.


================
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);
+
----------------
sivachandra wrote:
> Why are these functions taking an arg? Seems to me like they update `cur_pos` anyway?
Index mode needs its own parser for finding sizes of args, so these can't just work on `cur_pos` because sometimes there need to be two separate places being parsed from. Also it means that these functions don't implicitly modify any state, which I think makes them clearer.


================
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) {
----------------
sivachandra wrote:
> You should ideally implement a matcher but you can do it separately.
I will do that in a followup patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123339