Skip to content

Commit

Permalink
Disable usage of __is_cpp17_contiguous_iterator until libc++ rolls.
Browse files Browse the repository at this point in the history
Though names prefixed with `__` are reserved for implementation use,
specializing `__is_cpp17_contiguous_iterator` is currently the only way
to tell libc++ that `memcpy()` can be safe to use with a custom
iterator.  Without this workaround, there are noticeable regressions,
e.g. when using std::copy() with spans of trivially copyable types (see
https://crbug.com/994174).

However, https://reviews.llvm.org/D150801 renames this template to
`__libcpp_is_contiguous_iterator`, which blocks rolling past this libc++
revision. As a workaround, temporarily forward declare and specialize
both variants.
`
In addition, C++20 provides an official way for a custom iterator to opt
into these types of optimizations, using `iterator_concept`, so also go
ahead and opt `CheckedContiguousIterator` into using this.

Bug: 1449299
Bug: b/284031070
Change-Id: If4e667fca8d1475ce5ced76b6072134686d12f4a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4570684
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1150815}
  • Loading branch information
zetafunction authored and Chromium LUCI CQ committed May 30, 2023
1 parent 9da90d2 commit 9bfbbff
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 10 deletions.
25 changes: 24 additions & 1 deletion base/containers/checked_iterators.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ class CheckedContiguousIterator {
using pointer = T*;
using reference = T&;
using iterator_category = std::random_access_iterator_tag;
#if __cplusplus >= 202002L
using iterator_concept = std::contiguous_iterator_tag;
#endif

// Required for converting constructor below.
template <typename U>
Expand Down Expand Up @@ -239,14 +242,34 @@ using CheckedContiguousConstIterator = CheckedContiguousIterator<const T>;
// [1] https://wg21.link/iterator.concept.contiguous
// [2] https://wg21.link/std.iterator.tags
// [3] https://wg21.link/pointer.traits.optmem
namespace std {

#if defined(_LIBCPP_VERSION)

// TODO(crbug.com/1284275): Remove when C++20 is on by default, as the use
// of `iterator_concept` above should suffice.
_LIBCPP_BEGIN_NAMESPACE_STD

// TODO(crbug.com/1449299): https://reviews.llvm.org/D150801 renamed this from
// `__is_cpp17_contiguous_iterator` to `__libcpp_is_contiguous_iterator`. Clean
// up the old spelling after libc++ rolls.
template <typename T>
struct __is_cpp17_contiguous_iterator;
template <typename T>
struct __is_cpp17_contiguous_iterator<::base::CheckedContiguousIterator<T>>
: true_type {};

template <typename T>
struct __libcpp_is_contiguous_iterator;
template <typename T>
struct __libcpp_is_contiguous_iterator<::base::CheckedContiguousIterator<T>>
: true_type {};

_LIBCPP_END_NAMESPACE_STD

#endif

namespace std {

template <typename T>
struct pointer_traits<::base::CheckedContiguousIterator<T>> {
using pointer = ::base::CheckedContiguousIterator<T>;
Expand Down
29 changes: 20 additions & 9 deletions base/containers/checked_iterators_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,8 @@ TEST(CheckedContiguousIterator, ConvertingComparisonOperators) {

} // namespace base

// ChromeOS does not use the in-tree libc++, but rather a shared library that
// lags a bit behind.
// TODO(crbug.com/1166360): Enable this test on ChromeOS once the shared libc++
// is sufficiently modern.
#if defined(_LIBCPP_VERSION) && !BUILDFLAG(IS_NACL) && !BUILDFLAG(IS_CHROMEOS)
#if defined(_LIBCPP_VERSION)

namespace {

// Helper template that wraps an iterator and disables its dereference and
Expand All @@ -101,6 +98,8 @@ namespace {
template <typename Iterator>
struct DisableDerefAndIncr : Iterator {
using Iterator::Iterator;

// NOLINTNEXTLINE(google-explicit-constructor)
constexpr DisableDerefAndIncr(const Iterator& iter) : Iterator(iter) {}

constexpr typename Iterator::reference operator*() {
Expand All @@ -121,16 +120,28 @@ struct DisableDerefAndIncr : Iterator {

} // namespace

// Inherit `__is_cpp17_contiguous_iterator` and `pointer_traits` specializations
// from the base class.
namespace std {
// Inherit `__libcpp_is_contiguous_iterator` and `pointer_traits`
// specializations from the base class.

// TODO(crbug.com/1284275): Remove when C++20 is on by default, as the use
// of `iterator_concept` should suffice.
_LIBCPP_BEGIN_NAMESPACE_STD

// TODO(crbug.com/1449299): https://reviews.llvm.org/D150801 renamed this from
// `__is_cpp17_contiguous_iterator` to `__libcpp_is_contiguous_iterator`. Clean
// up the old spelling after libc++ rolls.
template <typename Iter>
struct __is_cpp17_contiguous_iterator<DisableDerefAndIncr<Iter>>
: __is_cpp17_contiguous_iterator<Iter> {};

template <typename Iter>
struct __libcpp_is_contiguous_iterator<DisableDerefAndIncr<Iter>>
: __libcpp_is_contiguous_iterator<Iter> {};

template <typename Iter>
struct pointer_traits<DisableDerefAndIncr<Iter>> : pointer_traits<Iter> {};
} // namespace std

_LIBCPP_END_NAMESPACE_STD

namespace base {

Expand Down

0 comments on commit 9bfbbff

Please sign in to comment.