[llvm-branch-commits] [libcxx] [libc++][format][7/7] Cleans up the buffer code. (PR #101876)
via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Sun Aug 4 02:06:42 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-libcxx
Author: Mark de Wever (mordante)
<details>
<summary>Changes</summary>
The internal format buffer code shipped with LLVM 19 is no longer used and removed. This also updates parts of the documentation to reflect its proper state.
---
Patch is 21.99 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/101876.diff
9 Files Affected:
- (modified) libcxx/include/__format/buffer.h (+32-286)
- (modified) libcxx/test/libcxx/transitive_includes/cxx03.csv (-1)
- (modified) libcxx/test/libcxx/transitive_includes/cxx11.csv (-1)
- (modified) libcxx/test/libcxx/transitive_includes/cxx14.csv (-1)
- (modified) libcxx/test/libcxx/transitive_includes/cxx17.csv (-1)
- (modified) libcxx/test/libcxx/transitive_includes/cxx20.csv (-2)
- (modified) libcxx/test/libcxx/transitive_includes/cxx23.csv (-2)
- (modified) libcxx/test/libcxx/transitive_includes/cxx26.csv (-2)
- (modified) libcxx/test/std/utilities/format/format.functions/format_tests.h (+1-1)
``````````diff
diff --git a/libcxx/include/__format/buffer.h b/libcxx/include/__format/buffer.h
index ca2334f93fd04..b0722535c6d87 100644
--- a/libcxx/include/__format/buffer.h
+++ b/libcxx/include/__format/buffer.h
@@ -39,7 +39,6 @@
#include <__type_traits/conditional.h>
#include <__utility/exception_guard.h>
#include <__utility/move.h>
-#include <climits> // LLVM-20 remove
#include <cstddef>
#include <stdexcept>
#include <string_view>
@@ -63,7 +62,7 @@ class _LIBCPP_HIDE_FROM_ABI __max_output_size {
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI explicit __max_output_size(size_t __max_size) : __max_size_{__max_size} {}
// This function adjusts the size of a (bulk) write operations. It ensures the
- // number of code units written by a __output_buffer never exceed
+ // number of code units written by a __output_buffer never exceeds
// __max_size_ code units.
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI size_t __write_request(size_t __code_units) {
size_t __result =
@@ -87,26 +86,7 @@ class _LIBCPP_HIDE_FROM_ABI __max_output_size {
/// type-erasure for the formatting functions. This reduces the number to
/// template instantiations.
///
-/// The design of the class is being changed to improve performance and do some
-/// code cleanups.
-/// The original design (as shipped up to LLVM-19) uses the following design:
-/// - There is an external object that connects the buffer to the output.
-/// - The class constructor stores a function pointer to a grow function and a
-/// type-erased pointer to the object that does the grow.
-/// - When writing data to the buffer would exceed the external buffer's
-/// capacity it requests the external buffer to flush its contents.
-///
-/// The new design tries to solve some issues with the current design:
-/// - The buffer used is a fixed-size buffer, benchmarking shows that using a
-/// dynamic allocated buffer has performance benefits.
-/// - Implementing P3107R5 "Permit an efficient implementation of std::print"
-/// is not trivial with the current buffers. Using the code from this series
-/// makes it trivial.
-///
-/// This class is ABI-tagged, still the new design does not change the size of
-/// objects of this class.
-///
-/// The new design is the following.
+/// The design is the following:
/// - There is an external object that connects the buffer to the output.
/// - This buffer object:
/// - inherits publicly from this class.
@@ -206,37 +186,19 @@ class _LIBCPP_TEMPLATE_VIS __output_buffer {
using value_type = _CharT;
using __prepare_write_type = void (*)(__output_buffer<_CharT>&, size_t);
- template <class _Tp> // Deprecated LLVM-19 function.
- _LIBCPP_HIDE_FROM_ABI explicit __output_buffer(_CharT* __ptr, size_t __capacity, _Tp* __obj)
- : __ptr_(__ptr),
- __capacity_(__capacity),
- __flush_([](_CharT* __p, size_t __n, void* __o) { static_cast<_Tp*>(__o)->__flush(__p, __n); }),
- __data_{.__version_llvm_20__ = false, .__obj_ = reinterpret_cast<uintptr_t>(__obj) >> 1} {}
-
- // New LLVM-20 function.
[[nodiscard]]
_LIBCPP_HIDE_FROM_ABI explicit __output_buffer(_CharT* __ptr, size_t __capacity, __prepare_write_type __prepare_write)
: __output_buffer{__ptr, __capacity, __prepare_write, nullptr} {}
- // New LLVM-20 function.
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI explicit __output_buffer(
_CharT* __ptr, size_t __capacity, __prepare_write_type __prepare_write, __max_output_size* __max_output_size)
: __ptr_(__ptr),
__capacity_(__capacity),
__prepare_write_(__prepare_write),
- __data_{.__version_llvm_20_ = true, .__max_output_size_ = reinterpret_cast<uintptr_t>(__max_output_size) >> 1} {
- }
-
- // Deprecated LLVM-19 function.
- _LIBCPP_HIDE_FROM_ABI void __reset(_CharT* __ptr, size_t __capacity) {
- __ptr_ = __ptr;
- __capacity_ = __capacity;
- }
+ __max_output_size_(__max_output_size) {}
- // New LLVM-20 function.
_LIBCPP_HIDE_FROM_ABI void __buffer_flused() { __size_ = 0; }
- // New LLVM-20 function.
_LIBCPP_HIDE_FROM_ABI void __buffer_moved(_CharT* __ptr, size_t __capacity) {
__ptr_ = __ptr;
__capacity_ = __capacity;
@@ -246,16 +208,18 @@ class _LIBCPP_TEMPLATE_VIS __output_buffer {
// Used in std::back_insert_iterator.
_LIBCPP_HIDE_FROM_ABI void push_back(_CharT __c) {
- if (__data_.__version_llvm_20_ && __data_.__max_output_size_ &&
- reinterpret_cast<__max_output_size*>(__data_.__max_output_size_ << 1)->__write_request(1) == 0)
+ if (__max_output_size_ && __max_output_size_->__write_request(1) == 0)
return;
+ _LIBCPP_ASSERT_INTERNAL(
+ __ptr_ && __size_ < __capacity_ && __available() >= 1, "attempted to write outside the buffer");
+
__ptr_[__size_++] = __c;
// Profiling showed flushing after adding is more efficient than flushing
// when entering the function.
if (__size_ == __capacity_)
- __flush(0);
+ __prepare_write(0);
}
/// Copies the input __str to the buffer.
@@ -276,30 +240,20 @@ class _LIBCPP_TEMPLATE_VIS __output_buffer {
// upper case. For integral these strings are short.
// TODO FMT Look at the improvements above.
size_t __n = __str.size();
- if (__data_.__version_llvm_20_ && __data_.__max_output_size_) {
- __n = reinterpret_cast<__max_output_size*>(__data_.__max_output_size_ << 1)->__write_request(__n);
+ if (__max_output_size_) {
+ __n = __max_output_size_->__write_request(__n);
if (__n == 0)
return;
}
- __flush_on_overflow(__n);
- if (__n < __capacity_) { // push_back requires the buffer to have room for at least one character (so use <).
- std::copy_n(__str.data(), __n, std::addressof(__ptr_[__size_]));
- __size_ += __n;
- return;
- }
-
- // The output doesn't fit in the internal buffer.
- // Copy the data in "__capacity_" sized chunks.
- _LIBCPP_ASSERT_INTERNAL(__size_ == 0, "the buffer should be flushed by __flush_on_overflow");
const _InCharT* __first = __str.data();
do {
- size_t __chunk = std::min(__n, __capacity_ - __size_);
+ __prepare_write(__n);
+ size_t __chunk = std::min(__n, __available());
std::copy_n(__first, __chunk, std::addressof(__ptr_[__size_]));
- __size_ = __chunk;
+ __size_ += __chunk;
__first += __chunk;
__n -= __chunk;
- __flush(__n);
} while (__n);
}
@@ -313,70 +267,39 @@ class _LIBCPP_TEMPLATE_VIS __output_buffer {
_LIBCPP_ASSERT_INTERNAL(__first <= __last, "not a valid range");
size_t __n = static_cast<size_t>(__last - __first);
- if (__data_.__version_llvm_20_ && __data_.__max_output_size_) {
- __n = reinterpret_cast<__max_output_size*>(__data_.__max_output_size_ << 1)->__write_request(__n);
+ if (__max_output_size_) {
+ __n = __max_output_size_->__write_request(__n);
if (__n == 0)
return;
}
- __flush_on_overflow(__n);
- if (__n < __capacity_) { // push_back requires the buffer to have room for at least one character (so use <).
- std::transform(__first, __last, std::addressof(__ptr_[__size_]), std::move(__operation));
- __size_ += __n;
- return;
- }
-
- // The output doesn't fit in the internal buffer.
- // Transform the data in "__capacity_" sized chunks.
- _LIBCPP_ASSERT_INTERNAL(__size_ == 0, "the buffer should be flushed by __flush_on_overflow");
do {
- size_t __chunk = std::min(__n, __capacity_ - __size_);
+ __prepare_write(__n);
+ size_t __chunk = std::min(__n, __available());
std::transform(__first, __first + __chunk, std::addressof(__ptr_[__size_]), __operation);
- __size_ = __chunk;
+ __size_ += __chunk;
__first += __chunk;
__n -= __chunk;
- __flush(__n);
} while (__n);
}
/// A \c fill_n wrapper.
_LIBCPP_HIDE_FROM_ABI void __fill(size_t __n, _CharT __value) {
- if (__data_.__version_llvm_20_ && __data_.__max_output_size_) {
- __n = reinterpret_cast<__max_output_size*>(__data_.__max_output_size_ << 1)->__write_request(__n);
+ if (__max_output_size_) {
+ __n = __max_output_size_->__write_request(__n);
if (__n == 0)
return;
}
- __flush_on_overflow(__n);
- if (__n < __capacity_) { // push_back requires the buffer to have room for at least one character (so use <).
- std::fill_n(std::addressof(__ptr_[__size_]), __n, __value);
- __size_ += __n;
- return;
- }
-
- // The output doesn't fit in the internal buffer.
- // Fill the buffer in "__capacity_" sized chunks.
- _LIBCPP_ASSERT_INTERNAL(__size_ == 0, "the buffer should be flushed by __flush_on_overflow");
do {
- size_t __chunk = std::min(__n, __capacity_);
+ __prepare_write(__n);
+ size_t __chunk = std::min(__n, __available());
std::fill_n(std::addressof(__ptr_[__size_]), __chunk, __value);
- __size_ = __chunk;
+ __size_ += __chunk;
__n -= __chunk;
- __flush(__n);
} while (__n);
}
- _LIBCPP_HIDE_FROM_ABI void __flush(size_t __size_hint) {
- if (!__data_.__version_llvm_20_) {
- // LLVM-19 code path
- __flush_(__ptr_, __size_, reinterpret_cast<void*>(__data_.__obj_ << 1));
- __size_ = 0;
- } else {
- // LLVM-20 code path
- __prepare_write_(*this, __size_hint + 1); // + 1 to always have space for the next time
- }
- }
-
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI size_t __capacity() const { return __capacity_; }
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI size_t __size() const { return __size_; }
@@ -384,94 +307,19 @@ class _LIBCPP_TEMPLATE_VIS __output_buffer {
_CharT* __ptr_;
size_t __capacity_;
size_t __size_{0};
- union {
- // LLVM-19 member
- void (*__flush_)(_CharT*, size_t, void*);
- // LLVM-20 member
- void (*__prepare_write_)(__output_buffer<_CharT>&, size_t);
- };
- static_assert(sizeof(__flush_) == sizeof(__prepare_write_), "The union is an ABI break.");
- static_assert(alignof(decltype(__flush_)) == alignof(decltype(__prepare_write_)), "The union is an ABI break.");
- // Note this code is quite ugly, but it can cleaned up once the LLVM-19 parts
- // of the code are removed.
- union {
- struct {
- uintptr_t __version_llvm_20__ : 1;
- uintptr_t __obj_ : CHAR_BIT * sizeof(void*) - 1;
- };
- struct {
- uintptr_t __version_llvm_20_ : 1;
- uintptr_t __max_output_size_ : CHAR_BIT * sizeof(__max_output_size*) - 1;
- };
- } __data_;
- static_assert(sizeof(__data_) == sizeof(void*), "The struct is an ABI break.");
- static_assert(alignof(decltype(__data_)) == alignof(void*), "The struct is an ABI break.");
-
- /// Flushes the buffer when the output operation would overflow the buffer.
- ///
- /// A simple approach for the overflow detection would be something along the
- /// lines:
- /// \code
- /// // The internal buffer is large enough.
- /// if (__n <= __capacity_) {
- /// // Flush when we really would overflow.
- /// if (__size_ + __n >= __capacity_)
- /// __flush();
- /// ...
- /// }
- /// \endcode
- ///
- /// This approach works for all cases but one:
- /// A __format_to_n_buffer_base where \ref __enable_direct_output is true.
- /// In that case the \ref __capacity_ of the buffer changes during the first
- /// \ref __flush. During that operation the output buffer switches from its
- /// __writer_ to its __storage_. The \ref __capacity_ of the former depends
- /// on the value of n, of the latter is a fixed size. For example:
- /// - a format_to_n call with a 10'000 char buffer,
- /// - the buffer is filled with 9'500 chars,
- /// - adding 1'000 elements would overflow the buffer so the buffer gets
- /// changed and the \ref __capacity_ decreases from 10'000 to
- /// __buffer_size (256 at the time of writing).
- ///
- /// This means that the \ref __flush for this class may need to copy a part of
- /// the internal buffer to the proper output. In this example there will be
- /// 500 characters that need this copy operation.
- ///
- /// Note it would be more efficient to write 500 chars directly and then swap
- /// the buffers. This would make the code more complex and \ref format_to_n is
- /// not the most common use case. Therefore the optimization isn't done.
- _LIBCPP_HIDE_FROM_ABI void __flush_on_overflow(size_t __n) {
- __n += __size_;
- if (__n >= __capacity_)
- __flush(__n - __capacity_ + 1);
- }
-};
-
-// ***** ***** ***** LLVM-19 classes ***** ***** *****
-
-/// A storage using an internal buffer.
-///
-/// This storage is used when writing a single element to the output iterator
-/// is expensive.
-template <__fmt_char_type _CharT>
-class _LIBCPP_TEMPLATE_VIS __internal_storage {
-public:
- _LIBCPP_HIDE_FROM_ABI _CharT* __begin() { return __buffer_; }
+ void (*__prepare_write_)(__output_buffer<_CharT>&, size_t);
+ __max_output_size* __max_output_size_;
- static constexpr size_t __buffer_size = 256 / sizeof(_CharT);
+ [[nodiscard]] _LIBCPP_HIDE_FROM_ABI size_t __available() const { return __capacity_ - __size_; }
-private:
- _CharT __buffer_[__buffer_size];
+ _LIBCPP_HIDE_FROM_ABI void __prepare_write(size_t __code_units) {
+ // Always have space for one additional code unit. This is a precondition of the push_back function.
+ __code_units += 1;
+ if (__available() < __code_units)
+ __prepare_write_(*this, __code_units + 1);
+ }
};
-/// A storage writing directly to the storage.
-///
-/// This requires the storage to be a contiguous buffer of \a _CharT.
-/// Since the output is directly written to the underlying storage this class
-/// is just an empty class.
-template <__fmt_char_type _CharT>
-class _LIBCPP_TEMPLATE_VIS __direct_storage {};
-
template <class _OutIt, class _CharT>
concept __enable_direct_output =
__fmt_char_type<_CharT> &&
@@ -480,40 +328,6 @@ concept __enable_direct_output =
// `#ifdef`.
|| same_as<_OutIt, __wrap_iter<_CharT*>>);
-/// Write policy for directly writing to the underlying output.
-template <class _OutIt, __fmt_char_type _CharT>
-class _LIBCPP_TEMPLATE_VIS __writer_direct {
-public:
- _LIBCPP_HIDE_FROM_ABI explicit __writer_direct(_OutIt __out_it) : __out_it_(__out_it) {}
-
- _LIBCPP_HIDE_FROM_ABI _OutIt __out_it() { return __out_it_; }
-
- _LIBCPP_HIDE_FROM_ABI void __flush(_CharT*, size_t __n) {
- // _OutIt can be a __wrap_iter<CharT*>. Therefore the original iterator
- // is adjusted.
- __out_it_ += __n;
- }
-
-private:
- _OutIt __out_it_;
-};
-
-/// Write policy for copying the buffer to the output.
-template <class _OutIt, __fmt_char_type _CharT>
-class _LIBCPP_TEMPLATE_VIS __writer_iterator {
-public:
- _LIBCPP_HIDE_FROM_ABI explicit __writer_iterator(_OutIt __out_it) : __out_it_{std::move(__out_it)} {}
-
- _LIBCPP_HIDE_FROM_ABI _OutIt __out_it() && { return std::move(__out_it_); }
-
- _LIBCPP_HIDE_FROM_ABI void __flush(_CharT* __ptr, size_t __n) {
- __out_it_ = std::ranges::copy_n(__ptr, __n, std::move(__out_it_)).out;
- }
-
-private:
- _OutIt __out_it_;
-};
-
/// Concept to see whether a \a _Container is insertable.
///
/// The concept is used to validate whether multiple calls to a
@@ -539,72 +353,6 @@ struct _LIBCPP_TEMPLATE_VIS __back_insert_iterator_container<back_insert_iterato
using type = _Container;
};
-/// Write policy for inserting the buffer in a container.
-template <class _Container>
-class _LIBCPP_TEMPLATE_VIS __writer_container {
-public:
- using _CharT = typename _Container::value_type;
-
- _LIBCPP_HIDE_FROM_ABI explicit __writer_container(back_insert_iterator<_Container> __out_it)
- : __container_{__out_it.__get_container()} {}
-
- _LIBCPP_HIDE_FROM_ABI auto __out_it() { return std::back_inserter(*__container_); }
-
- _LIBCPP_HIDE_FROM_ABI void __flush(_CharT* __ptr, size_t __n) {
- __container_->insert(__container_->end(), __ptr, __ptr + __n);
- }
-
-private:
- _Container* __container_;
-};
-
-/// Selects the type of the writer used for the output iterator.
-template <class _OutIt, class _CharT>
-class _LIBCPP_TEMPLATE_VIS __writer_selector {
- using _Container = typename __back_insert_iterator_container<_OutIt>::type;
-
-public:
- using type =
- conditional_t<!same_as<_Container, void>,
- __writer_container<_Container>,
- conditional_t<__enable_direct_output<_OutIt, _CharT>,
- __writer_direct<_OutIt, _CharT>,
- __writer_iterator<_OutIt, _CharT>>>;
-};
-
-/// The generic formatting buffer.
-template <class _OutIt, __fmt_char_type _CharT>
- requires(output_iterator<_OutIt, const _CharT&>)
-class _LIBCPP_TEMPLATE_VIS __format_buffer {
- using _Storage =
- conditional_t<__enable_direct_output<_OutIt, _CharT>, __direct_storage<_CharT>, __internal_storage<_CharT>>;
-
-public:
- _LIBCPP_HIDE_FROM_ABI explicit __format_buffer(_OutIt __out_it)
- requires(same_as<_Storage, __internal_storage<_CharT>>)
- : __output_(__storage_.__begin(), __storage_.__buffer_size, this), __writer_(std::move(__out_it)) {}
-
- _LIBCPP_HIDE_FROM_ABI explicit __format_buffer(_OutIt __out_it)
- requires(same_as<_Storage, __direct_storage<_CharT>>)
- : __output_(std::__unwrap_iter(__out_it), size_t(-1), this), __writer_(std::move(__out_it)) {}
-
- _LIBCPP_HIDE_FROM_ABI auto __make_output_iterator() { return __output_.__make_output_iterator(); }
-
- _LIBCPP_HIDE_FROM_ABI void __flush(_CharT* __ptr, size_t __n) { __writer_.__flush(__ptr, __n); }
-
- _LIBCPP_HIDE_FROM_ABI _OutIt __out_it() && {
- __output_.__flush(0);
- return std::move(__writer_).__out_it();
- }
-
-private:
- _LIBCPP_NO_UNIQUE_ADDRESS _Storage __storage_;
- __output_buffer<_CharT> __output_;
- typename __writer_selector<_OutIt, _CharT>::type __writer_;
-};
-
-// ***** ***** ***** LLVM-20 classes ***** ***** *****
-
// A dynamically growing buffer.
template <__fmt_char_type _CharT>
class _LIBCPP_TEMPLATE_VIS __allocating_buffer : public __output_buffer<_CharT> {
@@ -824,8 +572,6 @@ class _LIBCPP_TEMPLATE_VIS __formatted_size_buffer : private __output_buffer<_Ch
}
};
-// ***** ***** ***** LLVM-19 and LLVM-20 class ***** ***** *****
-
// A dynamically growing buffer intended to be used for retargeting a context.
//
// P2286 Formatting ranges adds range formatting support. It allows the user to
diff --git a/libcxx/test/libcxx/transitive_includes/cxx03.csv b/libcxx/test/libcxx/transitive_includes/cxx03.csv
index f6e7db17f413f..622fced5ffa40 100644
--- a/libcxx/test/libcxx/transitive_includes/cxx03.csv
+++ b/libcxx/test/libcxx/transitive_includes/cxx03.csv
@@ -860,7 +860,6 @@ thread atomic
thread cctype
thread cerrno
thread chrono
-thread climits
thread clocale
thread compare
thread cstddef
diff --git a/libcxx/test/libcxx/transitive_includes/cxx11.csv b/libcxx/test/libcxx/transitive_includes/cxx11.csv
index 752fea058e63b..11c3c1322c406 100644
--- a/libcxx/test/libcxx/transitive_includes/cxx11.csv
+++ b/libcxx/test/libcxx/transitive_includes/cxx11.csv
@@ -867,7 +867,6 @@ thread atomic
thread cctype
thread cerrno
thread chrono
-thread climits
thread clocale
thread compare
thread cstddef
diff --git a/libcxx/test/libcxx/transitive_includes/cxx14.csv b/libcxx/test/libcxx/transitive_includes/cxx14.csv
index 010f7e2fb82e9..666d5c3896467 100644
--- a/libcxx/test/libcxx/transitive_includes/cxx14.csv
+++ b/libcxx/test/libcxx/transitive_includes/cxx14.csv
@@ -870,7 +870,6 @@ thread atomic
thread cctype
thread cerrno
thread chrono
-thread climits
thread clocale
thread compare
thread cstddef
diff --git a/libcxx/test/libcxx/transitive_includes/cxx17.csv b/libcxx/test/libcxx/transitive_includes/cxx17.csv
index 64c2db3eef6f9..3a3aa5a894473 100644
--- a/libcxx/test/libcxx/transitive_includes/cxx17.csv
+++ b/libcxx/test/libcxx/transitive_includes/cxx17.csv
@@ -881,7 +881,6 @@ thread atomic
thread cctype
thread cerrno
thread chrono
-thread climits
thread clocale
thread compare
thread cstddef
diff --git a/libcxx/test/libcxx/transitive_includes/cxx20.csv b/libcxx/test/libcxx/transitive_includes/cxx20.csv
index a7ea7f8dddbea..982c2013e3417 100644
--- a/libcxx/test/libcxx/transitive_includes/cxx20.csv
+++ b/libcxx/test/libcxx/transitive_includes/cxx20.csv
@@ -276,7 +276,6 @@ filesystem version
format array
format cctype
format cerrno
-format climits
format clocale
f...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/101876
More information about the llvm-branch-commits
mailing list