This is an archive of the discontinued LLVM Phabricator instance.

[libc][NFC] add outline of printf
ClosedPublic

Authored by michaelrj on Mar 30 2022, 5:08 PM.

Details

Summary

This patch adds the headers for printf. It contains minimal actual code,
and is more intended to be used for design review. The code is not built
yet, and may have minor errors.

Diff Detail

Event Timeline

michaelrj created this revision.Mar 30 2022, 5:08 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 30 2022, 5:08 PM
michaelrj requested review of this revision.Mar 30 2022, 5:08 PM
lntue added inline comments.Mar 31 2022, 7:24 AM
libc/src/stdio/printf_files/core_structs.h
35

Is Float missing or not needed?

michaelrj marked an inline comment as done.Mar 31 2022, 10:08 AM
michaelrj added inline comments.
libc/src/stdio/printf_files/core_structs.h
35

Float is not needed. %f uses double and %Lf uses long double, but there's no float option.

michaelrj marked an inline comment as done.Mar 31 2022, 10:10 AM
sivachandra added inline comments.Mar 31 2022, 1:42 PM
libc/src/stdio/printf_files/core_structs.h
49

Type names should be of the form TypeName.

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

Do we really need an internal namespace here? You normally introduce an internal namespace to indicate that the interface declared in such a namespace is not to be used by clients of the higher level namespace. If we do need something internal, ISTM that the nesting should be reversed? As in, internal should be nested inside printf_core?

libc/src/stdio/printf_files/writer.h
21

Is this class envisioned to be a final class? If yes, then we should have documentation of the expectations. For example, what is output, raw_write etc. If not also, we should add documentation about how it is to be specialized.

34

Why is size required at all?

michaelrj updated this revision to Diff 419573.Mar 31 2022, 2:46 PM
michaelrj marked 4 inline comments as done.

add comments to explain functions

michaelrj added inline comments.Mar 31 2022, 2:47 PM
libc/src/stdio/printf_files/parser.h
19

I've removed the internal namespace and moved everything to printf_core.

libc/src/stdio/printf_files/writer.h
21

I was under the impression that virtual functions were disallowed in libc, so all classes were implicitly final.

I've added comments with more explanation on how this specialization works.

34

the number of chars written is the return value for printf, as well as the value written by %n.

sivachandra accepted this revision.Mar 31 2022, 3:13 PM

LGTM but we might tweak as we go.

libc/src/stdio/printf_files/writer.h
21

Couple of things:

  1. A class can be final without a virtual function.
  2. Going by your comments, this is class is a wrapper over the actual write streams. Which makes me feel it is a final class (as in, the intention is not to specialize it.) You don't need to mark it final though.
30

What is max_length?

39

These functions should be implemented inline to avoid multiple indirections?

This revision is now accepted and ready to land.Mar 31 2022, 3:13 PM
michaelrj updated this revision to Diff 419585.Mar 31 2022, 4:19 PM
michaelrj marked 2 inline comments as done.

make Writer a final class.

libc/src/stdio/printf_files/writer.h
21

I've marked it as final for clarity.

30

for snprintf there is a limit on the maximum number of characters that can be written. This represents that maximum length. In other cases it will be set to a default (probably size_t maximum).

39

Could you clarify what you mean by implementing this inline? This function won't just be calling the raw_write function. If length + chars_written > max_length then the length passed to raw_write will be max_length - chars_written, so that this doesn't overrun the buffer.

sivachandra added inline comments.Apr 1 2022, 11:40 AM
libc/src/stdio/printf_files/writer.h
39

This class is essentially a convenience wrapper to call raw_write, albeit with some logic before and after potentially. What I mean is that, if make these functions inline, then we avoid unnecessary function call indirections. You don't have to implement them in this patch of course.

This revision was automatically updated to reflect the committed changes.