The printf implmentation is made up of three main pieces, the parser,
the converter, and the writer. This patch adds the implementation for
the writer, as well as the function for writing to a string, along with
tests.
Details
- Reviewers
sivachandra lntue - Commits
- rGe072a123d3b2: [libc] add printf writer
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libc/src/stdio/printf_core/writer.cpp | ||
---|---|---|
23 | the check chars_written + length >= max_length is redundant. You can move it to an inline comment as reminding. |
libc/src/stdio/printf_core/CMakeLists.txt | ||
---|---|---|
28 | Indent. | |
libc/src/stdio/printf_core/string_writer.h | ||
24 | It seems to me like you want a stateful writer. So, why not just make it a class? | |
libc/src/stdio/printf_core/writer.h | ||
31 | Can you explain the need for max_length? It seems to me like it is something a specific writer, say StringWriter should care about, but not a generic writer? At this point, I would actually ask the need for a generic Writer class. It seems to me like all you need is a writer class which implements a method with a specific signature. For example, you want StringWriter like this: class StringWriter { private: ... // State as suggested in another comment public: // This method attempts to write |len| bytes from |data| and return the actual // number of bytes written. size_t write(const void *data, size_t len); }; FWIW, the File class already implements the write protocol. So, based on the printf function, sprintf or fprintf, one either uses the StringWriter or File. | |
33 | Is this required only for testing or do you need it for something else also? |
libc/src/stdio/printf_core/writer.h | ||
---|---|---|
31 | Actually, I am beginning think there is benefit with the generic Writer indirection. Consider the File example again - file writer might want to explicitly lock and use unlocked file methods to write to the file. Having a generic writer allows us to hide such details in the file writer. |
add StringWriter class and adjust tests to match
libc/src/stdio/printf_core/string_writer.h | ||
---|---|---|
24 | it felt like overkill for tracking only one variable, but if I'm moving max_len in there as well, then it makes more sense. | |
libc/src/stdio/printf_core/writer.h | ||
31 | File doesn't support all of the functions I need, specifically it doesn't have chars_written, which represents the number of characters written by this specific call to printf and is used both in the %n conversion and as the return value for all printf functions. I have moved max_length into the StringWriter class now though. | |
31 | I agree, and theoretically on platforms that don't care as much about code size we could have two string writer versions, one with a max length and one without, since that would improve performance. | |
33 | chars_written is the return value of printf, as well as the value to be written for %n |
libc/src/stdio/printf_core/string_writer.h | ||
---|---|---|
21 | Nit: May be call it available_capacity as that is how it is being used internally? | |
27 | Nit: Why not s/init_output/buffer and call the class member as buffer_ptr? | |
28 | Nit: Space between line 27 and 28. | |
libc/src/stdio/printf_core/writer.cpp | ||
21 | The + operator can potentially lead to a rollover so the check should be done the other way around? Couple of other points:
May be the raw_write should be a "raw" writer anyway. As in, it will just do the writing business and all higher level logic is up to this Writer class. | |
28 | The + operation here can also rollover. Is it OK? |
fix max_length still being in the core writer class, and the tests not using the max_length functionality in the string writer.
libc/src/stdio/printf_core/string_writer.h | ||
---|---|---|
27 | I went with cur_buffer since I felt that more accurately described how it was being used. | |
libc/src/stdio/printf_core/writer.cpp | ||
21 | The max length here should have been removed in the previous patch because I moved the max length logic to the string writer class, so yes it was redundant. As for if raw_write should be relatively logic free, I think it makes sense to have the length handled in the string writer, since the max length is only used for strings. Other than that it is relatively raw, only storing a pointer and the number of bytes left to write to it. | |
28 | chars_written is supposed to store the value that will be returned when printf completes. The return value of printf is the number of characters written by that call as represented by an int, or a negative number if there was an error. For this purpose, all chars_written needs to do is be at least as large as an int, and if there would be overflow, return a negative number to represent the error. For this purpose I've changed chars_written to an unsigned long long so that it will definitely be bigger than an int, and the actual printf code can return a negative number if it would overflow that int. Finally, unsigned long long is guaranteed to have a maximum value of at least 18446744073709551615 (2^64 - 1), and if any call to printf writes more than that many bytes I'm declaring it undefined behavior. |
libc/src/stdio/printf_core/string_writer.h | ||
---|---|---|
43 | yes. |
Indent.