This is an archive of the discontinued LLVM Phabricator instance.

[libc] refactor printf file writing
ClosedPublic

Authored by michaelrj on Jun 14 2022, 12:06 PM.

Details

Summary

Add return values to converter functions to allow for better error
handling when writing files. Also move the file writing code around to
be easier to read.

Diff Detail

Event Timeline

michaelrj created this revision.Jun 14 2022, 12:06 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 14 2022, 12:06 PM
michaelrj requested review of this revision.Jun 14 2022, 12:06 PM

Nit: You should probably keep the addition of vfprintf_internal in the other patch.

libc/src/stdio/printf_core/char_converter.h
22

Nit: If you make this macro take an argument, you can use like this:

RETURN_ON_ERROR(writer->write(...))
libc/src/stdio/printf_core/writer.cpp
21

Is this required now?

libc/src/stdio/printf_core/writer.h
32

Do we need this to be a signed integer now?

sivachandra added inline comments.Jun 15 2022, 1:23 AM
libc/src/stdio/printf_core/file_writer.h
24

Something I realized while reviewing the other patch: why should this internal class take a FILE* arg only to cast to the internal file data structure?

michaelrj marked 4 inline comments as done.

address comments and move macro to a shared location

libc/src/stdio/printf_core/file_writer.h
24

we have to cast it somewhere, and doing it inside vfprintf_internal means that we don't have to cast it in both fprintf and printf (and potentially also the v variants of those functions if those are added).

libc/src/stdio/printf_core/writer.h
32

It doesn't need to be an int, but it does make sense. Printf always returns an int, and that int is supposed to be the number of characters written, so storing the number of characters written in anything bigger doesn't really help.

sivachandra accepted this revision.Jun 15 2022, 11:25 AM
sivachandra added inline comments.
libc/src/stdio/printf_core/core_structs.h
84 ↗(On Diff #437252)

Nit: Put func in parenthesis: (func).

This revision is now accepted and ready to land.Jun 15 2022, 11:25 AM
michaelrj marked an inline comment as done.

fix nit before land

This revision was automatically updated to reflect the committed changes.