Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[libc++] Remove assumptions that std::array::iterator is a raw pointer #74624

Merged
merged 2 commits into from
Dec 18, 2023
Merged

[libc++] Remove assumptions that std::array::iterator is a raw pointer #74624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
860b13e
[libc++] Remove assumptions that std::array::iterator is a raw pointer
ldionne Dec 6, 2023
489fc76
Add missing constraints for iter_value_t
ldionne Dec 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 8 additions & 4 deletions libcxx/include/__format/buffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,10 @@ class _LIBCPP_TEMPLATE_VIS __output_buffer {
/// A std::transform wrapper.
///
/// Like @ref __copy it may need to do type conversion.
template <__fmt_char_type _InCharT, class _UnaryOperation>
_LIBCPP_HIDE_FROM_ABI void __transform(const _InCharT* __first, const _InCharT* __last, _UnaryOperation __operation) {
template <contiguous_iterator _Iterator,
class _UnaryOperation,
__fmt_char_type _InCharT = typename iterator_traits<_Iterator>::value_type>
_LIBCPP_HIDE_FROM_ABI void __transform(_Iterator __first, _Iterator __last, _UnaryOperation __operation) {
_LIBCPP_ASSERT_UNCATEGORIZED(__first <= __last, "not a valid range");

size_t __n = static_cast<size_t>(__last - __first);
Expand Down Expand Up @@ -590,8 +592,10 @@ class _LIBCPP_TEMPLATE_VIS __retarget_buffer {
__size_ += __n;
}

template <__fmt_char_type _InCharT, class _UnaryOperation>
_LIBCPP_HIDE_FROM_ABI void __transform(const _InCharT* __first, const _InCharT* __last, _UnaryOperation __operation) {
template <contiguous_iterator _Iterator,
class _UnaryOperation,
__fmt_char_type _InCharT = typename iterator_traits<_Iterator>::value_type>
_LIBCPP_HIDE_FROM_ABI void __transform(_Iterator __first, _Iterator __last, _UnaryOperation __operation) {
_LIBCPP_ASSERT_UNCATEGORIZED(__first <= __last, "not a valid range");

size_t __n = static_cast<size_t>(__last - __first);
Expand Down
37 changes: 23 additions & 14 deletions libcxx/include/__format/formatter_integral.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
#include <__format/format_error.h>
#include <__format/formatter_output.h>
#include <__format/parser_std_format_spec.h>
#include <__iterator/concepts.h>
#include <__iterator/iterator_traits.h>
#include <__memory/pointer_traits.h>
#include <__system_error/errc.h>
#include <__type_traits/make_unsigned.h>
#include <__utility/unreachable.h>
Expand Down Expand Up @@ -49,7 +52,9 @@ namespace __formatter {
// Generic
//

_LIBCPP_HIDE_FROM_ABI inline char* __insert_sign(char* __buf, bool __negative, __format_spec::__sign __sign) {
template <contiguous_iterator _Iterator>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These iterators should be constrained to have a char value_type the current constrains loosen these restrictions. (I'm fine with new concept contiguous_char_iterator if you prefer that.)

requires same_as<char, iter_value_t<_Iterator>>
_LIBCPP_HIDE_FROM_ABI inline _Iterator __insert_sign(_Iterator __buf, bool __negative, __format_spec::__sign __sign) {
if (__negative)
*__buf++ = '-';
else
Expand Down Expand Up @@ -148,14 +153,16 @@ _LIBCPP_HIDE_FROM_ABI auto __format_char(
// Integer
//

/** Wrapper around @ref to_chars, returning the output pointer. */
template <integral _Tp>
_LIBCPP_HIDE_FROM_ABI char* __to_buffer(char* __first, char* __last, _Tp __value, int __base) {
/** Wrapper around @ref to_chars, returning the output iterator. */
template <contiguous_iterator _Iterator, integral _Tp>
requires same_as<char, iter_value_t<_Iterator>>
_LIBCPP_HIDE_FROM_ABI _Iterator __to_buffer(_Iterator __first, _Iterator __last, _Tp __value, int __base) {
cjdb marked this conversation as resolved.
Show resolved Hide resolved
// TODO FMT Evaluate code overhead due to not calling the internal function
// directly. (Should be zero overhead.)
to_chars_result __r = std::to_chars(__first, __last, __value, __base);
to_chars_result __r = std::to_chars(std::to_address(__first), std::to_address(__last), __value, __base);
_LIBCPP_ASSERT_UNCATEGORIZED(__r.ec == errc(0), "Internal buffer too small");
return __r.ptr;
auto __diff = __r.ptr - std::to_address(__first);
return __first + __diff;
}

/**
Expand Down Expand Up @@ -203,9 +210,10 @@ consteval size_t __buffer_size() noexcept
+ 1; // Reserve space for the sign.
}

template <class _OutIt, class _CharT>
_LIBCPP_HIDE_FROM_ABI _OutIt __write_using_decimal_separators(_OutIt __out_it, const char* __begin, const char* __first,
const char* __last, string&& __grouping, _CharT __sep,
template <class _OutIt, contiguous_iterator _Iterator, class _CharT>
requires same_as<char, iter_value_t<_Iterator>>
_LIBCPP_HIDE_FROM_ABI _OutIt __write_using_decimal_separators(_OutIt __out_it, _Iterator __begin, _Iterator __first,
_Iterator __last, string&& __grouping, _CharT __sep,
__format_spec::__parsed_specifications<_CharT> __specs) {
int __size = (__first - __begin) + // [sign][prefix]
(__last - __first) + // data
Expand Down Expand Up @@ -269,22 +277,23 @@ _LIBCPP_HIDE_FROM_ABI _OutIt __write_using_decimal_separators(_OutIt __out_it, c



template <unsigned_integral _Tp, class _CharT, class _FormatContext>
template <unsigned_integral _Tp, contiguous_iterator _Iterator, class _CharT, class _FormatContext>
requires same_as<char, iter_value_t<_Iterator>>
_LIBCPP_HIDE_FROM_ABI typename _FormatContext::iterator __format_integer(
_Tp __value,
_FormatContext& __ctx,
__format_spec::__parsed_specifications<_CharT> __specs,
bool __negative,
char* __begin,
char* __end,
_Iterator __begin,
_Iterator __end,
const char* __prefix,
int __base) {
char* __first = __formatter::__insert_sign(__begin, __negative, __specs.__std_.__sign_);
_Iterator __first = __formatter::__insert_sign(__begin, __negative, __specs.__std_.__sign_);
if (__specs.__std_.__alternate_form_ && __prefix)
while (*__prefix)
*__first++ = *__prefix++;

char* __last = __formatter::__to_buffer(__first, __end, __value, __base);
_Iterator __last = __formatter::__to_buffer(__first, __end, __value, __base);

# ifndef _LIBCPP_HAS_NO_LOCALIZATION
if (__specs.__std_.__locale_specific_form_) {
Expand Down
34 changes: 22 additions & 12 deletions libcxx/include/__format/formatter_output.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,9 @@
#include <__format/unicode.h>
#include <__iterator/back_insert_iterator.h>
#include <__iterator/concepts.h>
#include <__iterator/iterator_traits.h> // iter_value_t
#include <__iterator/iterator_traits.h>
#include <__memory/addressof.h>
#include <__memory/pointer_traits.h>
#include <__utility/move.h>
#include <__utility/unreachable.h>
#include <cstddef>
Expand Down Expand Up @@ -110,26 +111,32 @@ _LIBCPP_HIDE_FROM_ABI auto __copy(basic_string_view<_CharT> __str, output_iterat
}
}

template <__fmt_char_type _CharT, __fmt_char_type _OutCharT = _CharT>
_LIBCPP_HIDE_FROM_ABI auto
__copy(const _CharT* __first, const _CharT* __last, output_iterator<const _OutCharT&> auto __out_it)
template <contiguous_iterator _Iterator,
__fmt_char_type _CharT = typename iterator_traits<_Iterator>::value_type,
__fmt_char_type _OutCharT = _CharT>
_LIBCPP_HIDE_FROM_ABI auto __copy(_Iterator __first, _Iterator __last, output_iterator<const _OutCharT&> auto __out_it)
-> decltype(__out_it) {
return __formatter::__copy(basic_string_view{__first, __last}, std::move(__out_it));
}

template <__fmt_char_type _CharT, __fmt_char_type _OutCharT = _CharT>
_LIBCPP_HIDE_FROM_ABI auto __copy(const _CharT* __first, size_t __n, output_iterator<const _OutCharT&> auto __out_it)
template <contiguous_iterator _Iterator,
__fmt_char_type _CharT = typename iterator_traits<_Iterator>::value_type,
__fmt_char_type _OutCharT = _CharT>
_LIBCPP_HIDE_FROM_ABI auto __copy(_Iterator __first, size_t __n, output_iterator<const _OutCharT&> auto __out_it)
-> decltype(__out_it) {
return __formatter::__copy(basic_string_view{__first, __n}, std::move(__out_it));
return __formatter::__copy(basic_string_view{std::to_address(__first), __n}, std::move(__out_it));
}

/// Transform wrapper.
///
/// This uses a "mass output function" of __format::__output_buffer when possible.
template <__fmt_char_type _CharT, __fmt_char_type _OutCharT = _CharT, class _UnaryOperation>
template <contiguous_iterator _Iterator,
__fmt_char_type _CharT = typename iterator_traits<_Iterator>::value_type,
__fmt_char_type _OutCharT = _CharT,
class _UnaryOperation>
_LIBCPP_HIDE_FROM_ABI auto
__transform(const _CharT* __first,
const _CharT* __last,
__transform(_Iterator __first,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal here was to reduce the number of instantiations to _CharT == char and _CharT == wchar_t. Would it be possible to adjust the caller to keep this code unchanged?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that, but the downside is that we'd have to use std::to_address and in that case we'd be manipulating raw pointers. So if we enable OOB-checked iterators for example, we wouldn't catch OOB accesses inside std::format. Do you still want me to make the change? I can, I just wanted to expose the tradeoff.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way we can do an OOB check ahead of time?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way we can do an OOB check ahead of time?

I think that might be possible, but I would approach it from the top down. So I would write this code in a generic fashion, and then I would ensure that it ends up only being called with (optionally OOB-checked) raw pointers to reduce the number of instantiations.

Also, at the end of the day, we're not instantiating this function with more unique types before/after this patch. We're just allowing the single type it's instantiated with to go from char* to array<char>::iterator seamlessly. The only additional weight we're adding is the concept constrained signature but it should only be instantiated with one iterator type regardless.

_Iterator __last,
output_iterator<const _OutCharT&> auto __out_it,
_UnaryOperation __operation) -> decltype(__out_it) {
if constexpr (std::same_as<decltype(__out_it), std::back_insert_iterator<__format::__output_buffer<_OutCharT>>>) {
Expand Down Expand Up @@ -260,8 +267,11 @@ __write(_Iterator __first,
return __formatter::__write(__first, __last, std::move(__out_it), __specs, __last - __first);
}

template <class _CharT, class _ParserCharT, class _UnaryOperation>
_LIBCPP_HIDE_FROM_ABI auto __write_transformed(const _CharT* __first, const _CharT* __last,
template <contiguous_iterator _Iterator,
class _CharT = typename iterator_traits<_Iterator>::value_type,
class _ParserCharT,
class _UnaryOperation>
_LIBCPP_HIDE_FROM_ABI auto __write_transformed(_Iterator __first, _Iterator __last,
output_iterator<const _CharT&> auto __out_it,
__format_spec::__parsed_specifications<_ParserCharT> __specs,
_UnaryOperation __op) -> decltype(__out_it) {
Expand Down
5 changes: 3 additions & 2 deletions libcxx/test/libcxx/input.output/iostream.format/print.fun/transcoding.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
// UNSUPPORTED: no-filesystem
// UNSUPPORTED: GCC-ALWAYS_INLINE-FIXME
// ADDITIONAL_COMPILE_FLAGS(has-fconstexpr-steps): -fconstexpr-steps=2000000

// <print>

Expand All @@ -32,9 +33,9 @@ constexpr void test(std::basic_string_view<CharT> expected, std::string_view inp
std::array<CharT, 1024> buffer;
std::ranges::fill(buffer, CharT('*'));

CharT* out = std::__unicode::__transcode(input.begin(), input.end(), buffer.data());
auto out = std::__unicode::__transcode(input.begin(), input.end(), buffer.begin());

assert(std::basic_string_view<CharT>(buffer.data(), out) == expected);
assert(std::basic_string_view<CharT>(buffer.begin(), out) == expected);

out = std::find_if(out, buffer.end(), [](CharT c) { return c != CharT('*'); });
assert(out == buffer.end());
Expand Down
18 changes: 8 additions & 10 deletions libcxx/test/std/algorithms/robust_against_proxy_iterators_lifetime_bugs.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,24 +547,22 @@ class ConstexprIterator {

#endif // TEST_STD_VER > 17

template <class T, std::size_t N = 32>
template <class T, std::size_t StorageSize = 32>
class Input {
using Array = std::array<T, N>;

std::size_t size_ = 0;
Array values_ = {};
T values_[StorageSize] = {};

public:
template <std::size_t N2>
TEST_CONSTEXPR_CXX20 Input(std::array<T, N2> from) {
static_assert(N2 <= N, "");
template <std::size_t N>
TEST_CONSTEXPR_CXX20 Input(std::array<T, N> from) {
static_assert(N <= StorageSize, "");

std::copy(from.begin(), from.end(), begin());
size_ = N2;
size_ = N;
}

TEST_CONSTEXPR_CXX20 typename Array::iterator begin() { return values_.begin(); }
TEST_CONSTEXPR_CXX20 typename Array::iterator end() { return values_.begin() + size_; }
TEST_CONSTEXPR_CXX20 T* begin() { return values_; }
TEST_CONSTEXPR_CXX20 T* end() { return values_ + size_; }
TEST_CONSTEXPR_CXX20 std::size_t size() const { return size_; }
};

Expand Down
4 changes: 0 additions & 4 deletions libcxx/test/std/containers/sequences/array/types.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,6 @@ int main(int, char**)
typedef std::array<T, 10> C;
static_assert((std::is_same<C::reference, T&>::value), "");
static_assert((std::is_same<C::const_reference, const T&>::value), "");
LIBCPP_STATIC_ASSERT((std::is_same<C::iterator, T*>::value), "");
LIBCPP_STATIC_ASSERT((std::is_same<C::const_iterator, const T*>::value), "");
test_iterators<C>();
static_assert((std::is_same<C::pointer, T*>::value), "");
static_assert((std::is_same<C::const_pointer, const T*>::value), "");
Expand All @@ -76,8 +74,6 @@ int main(int, char**)
typedef std::array<T, 0> C;
static_assert((std::is_same<C::reference, T&>::value), "");
static_assert((std::is_same<C::const_reference, const T&>::value), "");
LIBCPP_STATIC_ASSERT((std::is_same<C::iterator, T*>::value), "");
LIBCPP_STATIC_ASSERT((std::is_same<C::const_iterator, const T*>::value), "");
test_iterators<C>();
static_assert((std::is_same<C::pointer, T*>::value), "");
static_assert((std::is_same<C::const_pointer, const T*>::value), "");
Expand Down
5 changes: 4 additions & 1 deletion libcxx/test/std/containers/views/views.span/span.cons/iterator_sentinel.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,10 @@ class throw_operator_minus {
friend difference_type operator-(throw_operator_minus, throw_operator_minus) { throw 42; };

friend bool operator==(const throw_operator_minus& x, const throw_operator_minus& y) { return x.it_ == y.it_; }
friend auto operator<=>(const throw_operator_minus& x, const throw_operator_minus& y) { return x.it_ <=> y.it_; }
friend bool operator<(const throw_operator_minus& x, const throw_operator_minus& y) { return x.it_ < y.it_; }
friend bool operator>(const throw_operator_minus& x, const throw_operator_minus& y) { return x.it_ > y.it_; }
friend bool operator<=(const throw_operator_minus& x, const throw_operator_minus& y) { return x.it_ <= y.it_; }
friend bool operator>=(const throw_operator_minus& x, const throw_operator_minus& y) { return x.it_ >= y.it_; }
};

template <class It>
Expand Down