This is an archive of the discontinued LLVM Phabricator instance.

[libc][NFC] add index mode to printf parser
ClosedPublic

Authored by michaelrj on Apr 8 2022, 2:40 PM.

Details

Summary

This patch is a followup to the previous patch which implemented the
main printf parsing logic as well as sequential mode. This patch adds
index mode.

Diff Detail

Event Timeline

michaelrj created this revision.Apr 8 2022, 2:40 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 8 2022, 2:40 PM
michaelrj requested review of this revision.Apr 8 2022, 2:40 PM

Few nits inline, but can you add tests so that I understand how the new API is to be used and then I might have more comments.

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

Nit: Add a comment explain the difference between args_start vs args_cur. I think the naming is indicative enough, but would be nice if you can also add a note.

36

Nit: Move FLOAT_MASK below line 37 and add a comment explaining what it is.

62

Use inline_memset instead of this?

michaelrj marked 3 inline comments as done.

add unit tests and fix some problems that were exposed by those tests.

michaelrj edited the summary of this revision. (Show Details)Apr 25 2022, 11:38 AM
michaelrj updated this revision to Diff 425027.Apr 25 2022, 2:05 PM
michaelrj edited the summary of this revision. (Show Details)

fix a minor bug and add a comprehensive test to hopefully prevent future issues.

michaelrj updated this revision to Diff 426828.May 3 2022, 2:06 PM

clarify the type representations

I have not yet gone through the tests.

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

Nit: s/cur_size/cur_type_desc ?

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

Nit: Call it PrimaryType may be? The word super can be misleading because of other concepts like super classes.

38

Nit: Normally, things like this are named TypeDesc as a short for "type descriptor".

52

Nit: This is not an array of sizes anymore.

109

Looks like you can use a constexpr variable template instead of these functions?

140

Nit: s/set_size_arr/set_type_desc/ ?

167

Update the comment appropriately?

168

Nit: s/size_of_index/get_type_desc/ ?

michaelrj updated this revision to Diff 427473.May 5 2022, 3:15 PM
michaelrj marked 8 inline comments as done.

address comments, rename internal type representation to TypeDesc

sivachandra accepted this revision.May 6 2022, 10:22 AM
sivachandra added inline comments.
libc/src/stdio/printf_core/parser.h
121

Nit: set_type_desc to match get_type_desc below?

142

Update this comment with info about why this function exists. IIUC, it is adjusting to the current index in the varargs?

This revision is now accepted and ready to land.May 6 2022, 10:22 AM
michaelrj updated this revision to Diff 427697.May 6 2022, 11:44 AM
michaelrj marked 2 inline comments as done.

address final comments

This revision was automatically updated to reflect the committed changes.