[libc][printf] De-string-viewify writer internals. #170959
Open
+75
−67
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There are three flavors of WriteBuffer currently, all of which could be passed into printf_core::Writer class. It's a tricky class, since it chooses a flavor-specific logic either based on runtime dispatch (to save code size and prevent generating three versions of the entirety of printf_core), or based on template arguments (to avoid dealing with function pointers in codegen for FILL_BUFF_AND_DROP_OVERFLOW path. Refactor this somewhat convoluted logic to have three concrete subclasses inheriting from the templated base class, and use static polymorphism with reinterpret_cast to implement dispatching above. Now we can actually have flavor-specific fields, constructors, and methods (e.g. "flush_to_stream" is now a method of FlushingBuffer), and the code on the user side is cleaner: the complexity of enabling/disabling runtime-dispatch and using proper template arguments is now localized in writer.h. This code will need to be further templatized to support buffers of type wchar_t to implement swprintf() and friends. This change would make it (ever so slightly) easier.
This change doesn't yet modify Writer::write(string_view) public API, which has a lot of uses through printf_core. For wide-char write functions (like swprintf) we'd need to templatize WriteBuffer and Writer to support wide characrters. We'd need to either support helper cpp::string_view class to support wide-character strings, or replace them with pure pointer/size pairs. The latter option seems to be a more straightforward one, given that majority of printf_core operates on buffers anyway (and only constructs string_views right before invoking the buffer), and we don't have a use case internal wide-character string-views.
Member
|
@llvm/pr-subscribers-libc Author: Alexey Samsonov (vonosmas) ChangesThis change doesn't yet modify For wide-char write functions (like swprintf) we'd The latter option seems to be a more Full diff: https://github.com/llvm/llvm-project/pull/170959.diff 6 Files Affected:
diff --git a/libc/src/stdio/baremetal/printf.cpp b/libc/src/stdio/baremetal/printf.cpp
index 2fa9cf7c9f3cd..3329d4fecdad5 100644
--- a/libc/src/stdio/baremetal/printf.cpp
+++ b/libc/src/stdio/baremetal/printf.cpp
@@ -24,8 +24,9 @@ namespace LIBC_NAMESPACE_DECL {
namespace {
-LIBC_INLINE int stdout_write_hook(cpp::string_view new_str, void *) {
- write_to_stdout(new_str);
+LIBC_INLINE int stdout_write_hook(const char *new_str, size_t new_str_len,
+ void *) {
+ write_to_stdout({new_str, new_str_len});
return printf_core::WRITE_OK;
}
diff --git a/libc/src/stdio/baremetal/vprintf.cpp b/libc/src/stdio/baremetal/vprintf.cpp
index d89f26cd72b0a..30faa9ad54b8b 100644
--- a/libc/src/stdio/baremetal/vprintf.cpp
+++ b/libc/src/stdio/baremetal/vprintf.cpp
@@ -24,8 +24,9 @@ namespace LIBC_NAMESPACE_DECL {
namespace {
-LIBC_INLINE int stdout_write_hook(cpp::string_view new_str, void *) {
- write_to_stdout(new_str);
+LIBC_INLINE int stdout_write_hook(const char *new_str, size_t new_str_len,
+ void *) {
+ write_to_stdout({new_str, new_str_len});
return printf_core::WRITE_OK;
}
diff --git a/libc/src/stdio/printf_core/vasprintf_internal.h b/libc/src/stdio/printf_core/vasprintf_internal.h
index db6b95d49aaca..a36e9ca749305 100644
--- a/libc/src/stdio/printf_core/vasprintf_internal.h
+++ b/libc/src/stdio/printf_core/vasprintf_internal.h
@@ -18,9 +18,9 @@
namespace LIBC_NAMESPACE_DECL {
namespace printf_core {
-LIBC_INLINE int resize_overflow_hook(cpp::string_view new_str,
+LIBC_INLINE int resize_overflow_hook(const char *new_str, size_t new_str_len,
ResizingBuffer *wb) {
- size_t new_size = new_str.size() + wb->buff_cur;
+ size_t new_size = new_str_len + wb->buff_cur;
const bool isBuffOnStack = (wb->buff == wb->init_buff);
char *new_buff = static_cast<char *>(
isBuffOnStack ? malloc(new_size + 1)
@@ -33,7 +33,7 @@ LIBC_INLINE int resize_overflow_hook(cpp::string_view new_str,
if (isBuffOnStack)
inline_memcpy(new_buff, wb->buff, wb->buff_cur);
wb->buff = new_buff;
- inline_memcpy(wb->buff + wb->buff_cur, new_str.data(), new_str.size());
+ inline_memcpy(wb->buff + wb->buff_cur, new_str, new_str_len);
wb->buff_cur = new_size;
wb->buff_len = new_size;
return printf_core::WRITE_OK;
diff --git a/libc/src/stdio/printf_core/vfprintf_internal.h b/libc/src/stdio/printf_core/vfprintf_internal.h
index 321b0693ad339..b2b282fbf7b78 100644
--- a/libc/src/stdio/printf_core/vfprintf_internal.h
+++ b/libc/src/stdio/printf_core/vfprintf_internal.h
@@ -62,19 +62,20 @@ LIBC_INLINE FileIOResult fwrite_unlocked(const void *ptr, size_t size,
namespace printf_core {
-LIBC_INLINE int file_write_hook(cpp::string_view new_str, void *fp) {
+LIBC_INLINE int file_write_hook(const char *new_str, size_t new_str_len,
+ void *fp) {
::FILE *target_file = reinterpret_cast<::FILE *>(fp);
// Write new_str to the target file. The logic preventing a zero-length write
// is in the writer, so we don't check here.
- auto write_result = internal::fwrite_unlocked(new_str.data(), sizeof(char),
- new_str.size(), target_file);
+ auto write_result = internal::fwrite_unlocked(new_str, sizeof(char),
+ new_str_len, target_file);
// Propagate actual system error in FileIOResult.
if (write_result.has_error())
return -write_result.error;
// In case short write occured or error was not set on FileIOResult for some
// reason.
- if (write_result.value != new_str.size() ||
+ if (write_result.value != new_str_len ||
internal::ferror_unlocked(target_file))
return FILE_WRITE_ERROR;
diff --git a/libc/src/stdio/printf_core/writer.h b/libc/src/stdio/printf_core/writer.h
index cb45b105597d1..1eeffee2f5dde 100644
--- a/libc/src/stdio/printf_core/writer.h
+++ b/libc/src/stdio/printf_core/writer.h
@@ -41,7 +41,8 @@ template <WriteMode write_mode> struct Mode {
template <WriteMode write_mode> class Writer;
template <WriteMode write_mode> struct WriteBuffer {
- char *buff;
+ using CharType = char;
+ CharType *buff;
size_t buff_len;
size_t buff_cur = 0;
// The current writing mode in case the user wants runtime dispatch of the
@@ -49,7 +50,7 @@ template <WriteMode write_mode> struct WriteBuffer {
[[maybe_unused]] WriteMode write_mode_;
protected:
- LIBC_INLINE WriteBuffer(char *buff, size_t buff_len, WriteMode mode)
+ LIBC_INLINE WriteBuffer(CharType *buff, size_t buff_len, WriteMode mode)
: buff(buff), buff_len(buff_len), write_mode_(mode) {}
private:
@@ -57,24 +58,26 @@ template <WriteMode write_mode> struct WriteBuffer {
// The overflow_write method will handle the case when adding new_str to
// the buffer would overflow it. Specific actions will depend on the buffer
// type / write_mode.
- LIBC_INLINE int overflow_write(cpp::string_view new_str);
+ LIBC_INLINE int overflow_write(const CharType *new_str, size_t new_str_len);
};
// Buffer variant that discards characters that don't fit into the buffer.
struct DropOverflowBuffer
: public WriteBuffer<Mode<WriteMode::FILL_BUFF_AND_DROP_OVERFLOW>::value> {
- LIBC_INLINE DropOverflowBuffer(char *buff, size_t buff_len)
+ LIBC_INLINE DropOverflowBuffer(CharType *buff, size_t buff_len)
: WriteBuffer<Mode<WriteMode::FILL_BUFF_AND_DROP_OVERFLOW>::value>(
buff, buff_len, WriteMode::FILL_BUFF_AND_DROP_OVERFLOW) {}
- LIBC_INLINE int fill_remaining_to_buff(cpp::string_view new_str) {
+ LIBC_INLINE int fill_remaining_to_buff(const CharType *new_str,
+ size_t new_str_len) {
if (buff_cur < buff_len) {
- size_t bytes_to_write = buff_len - buff_cur;
- if (bytes_to_write > new_str.size()) {
- bytes_to_write = new_str.size();
+ size_t chars_to_write = buff_len - buff_cur;
+ if (chars_to_write > new_str_len) {
+ chars_to_write = new_str_len;
}
- inline_memcpy(buff + buff_cur, new_str.data(), bytes_to_write);
- buff_cur += bytes_to_write;
+ inline_memcpy(buff + buff_cur, new_str,
+ chars_to_write * sizeof(CharType));
+ buff_cur += chars_to_write;
}
return WRITE_OK;
}
@@ -84,27 +87,27 @@ struct DropOverflowBuffer
struct FlushingBuffer
: public WriteBuffer<Mode<WriteMode::FLUSH_TO_STREAM>::value> {
// The stream writer will be called when the buffer is full. It will be passed
- // string_views to write to the stream.
- using StreamWriter = int (*)(cpp::string_view, void *);
+ // buffers to write, and the target provided at construction time.
+ using StreamWriter = int (*)(const CharType *, size_t, void *);
const StreamWriter stream_writer;
void *output_target;
- LIBC_INLINE FlushingBuffer(char *buff, size_t buff_len, StreamWriter hook,
+ LIBC_INLINE FlushingBuffer(CharType *buff, size_t buff_len, StreamWriter hook,
void *target)
: WriteBuffer<Mode<WriteMode::FLUSH_TO_STREAM>::value>(
buff, buff_len, WriteMode::FLUSH_TO_STREAM),
stream_writer(hook), output_target(target) {}
- // Flushes the entire current buffer to stream, followed by the new_str (if
- // non-empty).
- LIBC_INLINE int flush_to_stream(cpp::string_view new_str) {
+ // Flushes the entire current buffer to stream, followed by the new_str
+ // buffer.
+ LIBC_INLINE int flush_to_stream(const CharType *new_str, size_t new_str_len) {
if (buff_cur > 0) {
- int retval = stream_writer({buff, buff_cur}, output_target);
+ int retval = stream_writer(buff, buff_cur, output_target);
if (retval < 0)
return retval;
}
- if (new_str.size() > 0) {
- int retval = stream_writer(new_str, output_target);
+ if (new_str_len > 0) {
+ int retval = stream_writer(new_str, new_str_len, output_target);
if (retval < 0)
return retval;
}
@@ -112,66 +115,73 @@ struct FlushingBuffer
return WRITE_OK;
}
- LIBC_INLINE int flush_to_stream() { return flush_to_stream({}); }
+ LIBC_INLINE int flush_to_stream() { return flush_to_stream(nullptr, 0); }
};
// Buffer variant that calls a resizing callback when it gets full.
struct ResizingBuffer
: public WriteBuffer<Mode<WriteMode::RESIZE_AND_FILL_BUFF>::value> {
- using ResizeWriter = int (*)(cpp::string_view, ResizingBuffer *);
+ using ResizeWriter = int (*)(const CharType *, size_t, ResizingBuffer *);
const ResizeWriter resize_writer;
- const char *init_buff; // for checking when resize.
+ const CharType *init_buff; // for checking when resize.
- LIBC_INLINE ResizingBuffer(char *buff, size_t buff_len, ResizeWriter hook)
+ LIBC_INLINE ResizingBuffer(CharType *buff, size_t buff_len, ResizeWriter hook)
: WriteBuffer<Mode<WriteMode::RESIZE_AND_FILL_BUFF>::value>(
buff, buff_len, WriteMode::RESIZE_AND_FILL_BUFF),
resize_writer(hook), init_buff(buff) {}
// Invokes the callback that is supposed to resize the buffer and make
// it large enough to fit the new_str addition.
- LIBC_INLINE int resize_and_write(cpp::string_view new_str) {
- return resize_writer(new_str, this);
+ LIBC_INLINE int resize_and_write(const CharType *new_str,
+ size_t new_str_len) {
+ return resize_writer(new_str, new_str_len, this);
}
};
template <>
LIBC_INLINE int WriteBuffer<WriteMode::RUNTIME_DISPATCH>::overflow_write(
- cpp::string_view new_str) {
+ const CharType *new_str, size_t new_str_len) {
if (write_mode_ == WriteMode::FILL_BUFF_AND_DROP_OVERFLOW)
return reinterpret_cast<DropOverflowBuffer *>(this)->fill_remaining_to_buff(
- new_str);
+ new_str, new_str_len);
else if (write_mode_ == WriteMode::FLUSH_TO_STREAM)
- return reinterpret_cast<FlushingBuffer *>(this)->flush_to_stream(new_str);
+ return reinterpret_cast<FlushingBuffer *>(this)->flush_to_stream(
+ new_str, new_str_len);
else if (write_mode_ == WriteMode::RESIZE_AND_FILL_BUFF)
- return reinterpret_cast<ResizingBuffer *>(this)->resize_and_write(new_str);
+ return reinterpret_cast<ResizingBuffer *>(this)->resize_and_write(
+ new_str, new_str_len);
__builtin_unreachable();
}
template <>
LIBC_INLINE int
WriteBuffer<WriteMode::FILL_BUFF_AND_DROP_OVERFLOW>::overflow_write(
- cpp::string_view new_str) {
+ const CharType *new_str, size_t new_str_len) {
return reinterpret_cast<DropOverflowBuffer *>(this)->fill_remaining_to_buff(
- new_str);
+ new_str, new_str_len);
}
template <>
-LIBC_INLINE int WriteBuffer<WriteMode::FLUSH_TO_STREAM>::overflow_write(
- cpp::string_view new_str) {
- return reinterpret_cast<FlushingBuffer *>(this)->flush_to_stream(new_str);
+LIBC_INLINE int
+WriteBuffer<WriteMode::FLUSH_TO_STREAM>::overflow_write(const CharType *new_str,
+ size_t new_str_len) {
+ return reinterpret_cast<FlushingBuffer *>(this)->flush_to_stream(new_str,
+ new_str_len);
}
template <>
LIBC_INLINE int WriteBuffer<WriteMode::RESIZE_AND_FILL_BUFF>::overflow_write(
- cpp::string_view new_str) {
- return reinterpret_cast<ResizingBuffer *>(this)->resize_and_write(new_str);
+ const CharType *new_str, size_t new_str_len) {
+ return reinterpret_cast<ResizingBuffer *>(this)->resize_and_write(
+ new_str, new_str_len);
}
template <WriteMode write_mode> class Writer final {
+ using CharType = char;
WriteBuffer<write_mode> &wb;
size_t chars_written = 0;
- LIBC_INLINE int pad(char new_char, size_t length) {
+ LIBC_INLINE int pad(CharType new_char, size_t length) {
// First, fill as much of the buffer as possible with the padding char.
size_t written = 0;
const size_t buff_space = wb.buff_len - wb.buff_cur;
@@ -184,17 +194,15 @@ template <WriteMode write_mode> class Writer final {
// Next, overflow write the rest of length using the mini_buff.
constexpr size_t MINI_BUFF_SIZE = 64;
- char mini_buff[MINI_BUFF_SIZE];
+ CharType mini_buff[MINI_BUFF_SIZE];
inline_memset(mini_buff, new_char, MINI_BUFF_SIZE);
- cpp::string_view mb_string_view(mini_buff, MINI_BUFF_SIZE);
while (written + MINI_BUFF_SIZE < length) {
- int result = wb.overflow_write(mb_string_view);
+ int result = wb.overflow_write(mini_buff, MINI_BUFF_SIZE);
if (result != WRITE_OK)
return result;
written += MINI_BUFF_SIZE;
}
- cpp::string_view mb_substr = mb_string_view.substr(0, length - written);
- return wb.overflow_write(mb_substr);
+ return wb.overflow_write(mini_buff, length - written);
}
public:
@@ -202,6 +210,7 @@ template <WriteMode write_mode> class Writer final {
// Takes a string, copies it into the buffer if there is space, else passes it
// to the overflow mechanism to be handled separately.
+ // TODO: Migrate all callers away from string_view to CharType*/size_t pair.
LIBC_INLINE int write(cpp::string_view new_string) {
chars_written += new_string.size();
if (LIBC_LIKELY(wb.buff_cur + new_string.size() <= wb.buff_len)) {
@@ -210,18 +219,17 @@ template <WriteMode write_mode> class Writer final {
wb.buff_cur += new_string.size();
return WRITE_OK;
}
- return wb.overflow_write(new_string);
+ return wb.overflow_write(new_string.data(), new_string.size());
}
// Takes a char and a length, memsets the next length characters of the buffer
// if there is space, else calls pad which will loop and call the overflow
// mechanism on a secondary buffer.
- LIBC_INLINE int write(char new_char, size_t length) {
+ LIBC_INLINE int write(CharType new_char, size_t length) {
chars_written += length;
if (LIBC_LIKELY(wb.buff_cur + length <= wb.buff_len)) {
- inline_memset(wb.buff + wb.buff_cur, static_cast<unsigned char>(new_char),
- length);
+ inline_memset(wb.buff + wb.buff_cur, new_char, length);
wb.buff_cur += length;
return WRITE_OK;
}
@@ -230,15 +238,14 @@ template <WriteMode write_mode> class Writer final {
// Takes a char, copies it into the buffer if there is space, else passes it
// to the overflow mechanism to be handled separately.
- LIBC_INLINE int write(char new_char) {
+ LIBC_INLINE int write(CharType new_char) {
chars_written += 1;
if (LIBC_LIKELY(wb.buff_cur + 1 <= wb.buff_len)) {
wb.buff[wb.buff_cur] = new_char;
wb.buff_cur += 1;
return WRITE_OK;
}
- cpp::string_view char_string_view(&new_char, 1);
- return wb.overflow_write(char_string_view);
+ return wb.overflow_write(&new_char, 1);
}
LIBC_INLINE size_t get_chars_written() { return chars_written; }
diff --git a/libc/test/src/stdio/printf_core/writer_test.cpp b/libc/test/src/stdio/printf_core/writer_test.cpp
index e0128505b2766..8f14e38aab922 100644
--- a/libc/test/src/stdio/printf_core/writer_test.cpp
+++ b/libc/test/src/stdio/printf_core/writer_test.cpp
@@ -8,13 +8,11 @@
#include "src/stdio/printf_core/writer.h"
-#include "src/__support/CPP/string_view.h"
#include "src/string/memory_utils/inline_memcpy.h"
#include "test/UnitTest/Test.h"
namespace {
-using LIBC_NAMESPACE::cpp::string_view;
using LIBC_NAMESPACE::printf_core::DropOverflowBuffer;
using LIBC_NAMESPACE::printf_core::FlushingBuffer;
using LIBC_NAMESPACE::printf_core::WriteMode;
@@ -196,17 +194,17 @@ struct OutBuff {
size_t cur_pos = 0;
};
-int copy_to_out(string_view new_str, void *raw_out_buff) {
- if (new_str.size() == 0) {
+int copy_to_out(const char *new_str, size_t new_str_len, void *raw_out_buff) {
+ if (new_str_len == 0) {
return 0;
}
OutBuff *out_buff = reinterpret_cast<OutBuff *>(raw_out_buff);
- LIBC_NAMESPACE::inline_memcpy(out_buff->out_str + out_buff->cur_pos,
- new_str.data(), new_str.size());
+ LIBC_NAMESPACE::inline_memcpy(out_buff->out_str + out_buff->cur_pos, new_str,
+ new_str_len);
- out_buff->cur_pos += new_str.size();
+ out_buff->cur_pos += new_str_len;
return 0;
}
|
Contributor
Author
|
This is stacked on top of PR #169089 , to reduce the usage of cpp::string_view in Writer / WriteBuffer to simplify further templatization. |
Contributor
|
I very much think this is the wrong direction. Using span / string_view style types rather than raw pointer / size pairs is something I really think we should aspire to stick to and move more outlier code towards. In the long run, more Hand-in-Hand work with libc++ will probably give us fuller string_view / span implementations to use directly in our hermetic namespace without reimplementing them all ourselves as we have now in On the contrary, I think reverting from span/view-style types to an earlier era's C conventions is itself a slippery slope of backsliding to lose the benefits of our rigorous and modern C++ approach to libc implementation internals. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change doesn't yet modify
Writer::write(string_view) public API, which
has a lot of uses through printf_core.
For wide-char write functions (like swprintf) we'd
need to templatize WriteBuffer and Writer to
support wide characrters. We'd need to either
support helper cpp::string_view class to support
wide-character strings, or replace them with pure
pointer/size pairs.
The latter option seems to be a more
straightforward one, given that majority of printf_core operates on buffers anyway
(and only constructs string_views right
before invoking the buffer), and we don't
have a use case internal wide-character
string-views.