This is an archive of the discontinued LLVM Phabricator instance.

[libc][NFC] implement printf parser
ClosedPublic

Authored by michaelrj on Apr 7 2022, 2:26 PM.

Details

Summary

This patch adds the sequential mode implementation of the printf parser,
as well as unit tests for it. In addition it adjusts the surrounding
files to accomodate changes in the design found in the implementation
process.

Diff Detail

Event Timeline

michaelrj created this revision.Apr 7 2022, 2:26 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 7 2022, 2:26 PM
michaelrj requested review of this revision.Apr 7 2022, 2:26 PM
michaelrj updated this revision to Diff 421354.Apr 7 2022, 3:17 PM

clarify tests.

lntue added inline comments.Apr 7 2022, 7:17 PM
libc/src/stdio/printf_files/parser.cpp
33 ↗(On Diff #421354)

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.

libc/src/stdio/printf_files/parser.h
0

Do you only need const & here?

sivachandra added inline comments.Apr 8 2022, 9:39 AM
libc/src/stdio/printf_files/CMakeLists.txt
1 ↗(On Diff #421354)

It seems to me like directory should be named printf without the _files suffix. Normally, the _files kind of prefixes/suffixes indicate data files and not source files.

libc/src/stdio/printf_files/parser.cpp
46 ↗(On Diff #421354)

I am very likely missing something obvious here: where is parse_index defined?

libc/src/stdio/printf_files/parser.h
0

I think this array is storing the sizes of the var-args. Add a comment saying so. Also, what can be optional and why?

michaelrj updated this revision to Diff 421579.Apr 8 2022, 10:32 AM
michaelrj marked 5 inline comments as done.

rename printf_files -> printf_core
address comments.

libc/src/stdio/printf_files/CMakeLists.txt
1 ↗(On Diff #421354)

I'm worried about name collision between the function and the folder if they're both just named printf, so I changed the name to printf_core to match the namespace.

libc/src/stdio/printf_files/parser.cpp
46 ↗(On Diff #421354)

parse_index is an index mode function not implemented in this patch. It's in the next patch which contains the index mode code.

libc/src/stdio/printf_files/parser.h
0

args can't be const because it's a container for a va_list which can't be const.

0

The size array is optional in the sense that it's only needed in index mode, and can be disabled in sequential mode.

sivachandra accepted this revision.Apr 8 2022, 11:47 AM

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

libc/src/stdio/printf_core/core_structs.h
20–21

Add a comment to explain the style exception here.

36

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?

39

Nit: When we have bit flags, 0 is the implicit no flag value. You don't need a symbolic name.

libc/src/stdio/printf_core/parser.cpp
21

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.

libc/src/stdio/printf_core/parser.h
32–33

Seems to me like FLOAT_MASK and SIZE_MASK are unused. So, remove them until they are actually used somewhere?

60

Why are these functions taking an arg? Seems to me like they update cur_pos anyway?

libc/test/src/stdio/printf_core/parser_test.cpp
20

You should ideally implement a matcher but you can do it separately.

This revision is now accepted and ready to land.Apr 8 2022, 11:47 AM
michaelrj updated this revision to Diff 421631.Apr 8 2022, 2:07 PM
michaelrj marked 6 inline comments as done.

Address comments.

michaelrj added inline comments.Apr 8 2022, 2:16 PM
libc/src/stdio/printf_core/parser.cpp
21

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.

libc/src/stdio/printf_core/parser.h
60

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.

libc/test/src/stdio/printf_core/parser_test.cpp
20

I will do that in a followup patch.

This revision was automatically updated to reflect the committed changes.