Skip to content

Implement LWG-3392 ranges::distance() cannot be used on a move-only iterator with a sized sentinel #2421

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

Merged
Stephan T. Lavavej (StephanTLavavej) merged 3 commits into from
Jan 22, 2022
Merged

Implement LWG-3392 ranges::distance() cannot be used on a move-only iterator with a sized sentinel #2421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3f152b1
Implement LWG-3392
CaseyCarter Dec 11, 2021
882c76b
Wrap overly long line in a `clang-format off` region.
StephanTLavavej Dec 16, 2021
2a99e8a
miscco's review comment
CaseyCarter Dec 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 16 additions & 18 deletions stl/inc/xutility
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2842,15 +2842,19 @@ namespace ranges {
public:
using _Not_quite_object::_Not_quite_object;

// clang-format off
template <input_or_output_iterator _It, sentinel_for<_It> _Se>
_NODISCARD constexpr iter_difference_t<_It> operator()(_It _First, _Se _Last) const
noexcept(_Nothrow_distance<_It, _Se>) /* strengthened */ {
if constexpr (sized_sentinel_for<_Se, _It>) {
return _Last - _First;
} else {
_Adl_verify_range(_First, _Last);
return _Distance_unchecked(_Get_unwrapped(_STD move(_First)), _Get_unwrapped(_STD move(_Last)));
}
requires (!sized_sentinel_for<_Se, _It>)
_NODISCARD constexpr iter_difference_t<_It> operator()(_It _First, _Se _Last) const {
// clang-format on
_Adl_verify_range(_First, _Last);
return _Distance_unchecked(_Get_unwrapped(_STD move(_First)), _Get_unwrapped(_STD move(_Last)));
}

template <input_or_output_iterator _It, sized_sentinel_for<_It> _Se>
_NODISCARD constexpr iter_difference_t<_It> operator()(const _It& _First, const _Se& _Last) const
noexcept(noexcept(_Last - _First)) /* strengthened */ {
return _Last - _First;
}

template <range _Rng>
Expand All @@ -2866,8 +2870,7 @@ namespace ranges {
private:
template <class _It, class _Se>
_NODISCARD static constexpr iter_difference_t<_It> _Distance_unchecked(_It _First, const _Se _Last) noexcept(
_Nothrow_distance<_It, _Se>) {
_STL_INTERNAL_STATIC_ASSERT(input_or_output_iterator<_It>);
noexcept(++_First != _Last)) {
_STL_INTERNAL_STATIC_ASSERT(sentinel_for<_Se, _It>);

iter_difference_t<_It> _Count = 0;
Expand All @@ -2878,15 +2881,10 @@ namespace ranges {
return _Count;
}

template <class _It, class _Se>
static constexpr bool _Nothrow_distance = noexcept(
noexcept(++_STD declval<_Unwrapped_t<_It>&>() != _STD declval<const _Unwrapped_t<_Se>&>()));
template <class _It, sized_sentinel_for<_It> _Se>
static constexpr bool _Nothrow_distance<_It, _Se> = noexcept(
noexcept(_STD declval<_Se&>() - _STD declval<_It&>()));

template <class _Rng>
static constexpr bool _Nothrow_size = _Nothrow_distance<iterator_t<_Rng>, sentinel_t<_Rng>>;
static constexpr bool _Nothrow_size = noexcept(
_Distance_unchecked(_Ubegin(_STD declval<_Rng&>()), _Uend(_STD declval<_Rng&>())));

template <sized_range _Rng>
static constexpr bool _Nothrow_size<_Rng> = noexcept(_RANGES size(_STD declval<_Rng&>()));
};
Expand Down
154 changes: 113 additions & 41 deletions tests/std/tests/P0896R4_ranges_iterator_machinery/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1877,10 +1877,11 @@ namespace iter_ops {

enum class sized { no, yes };
enum class assign { no, yes };
enum class nothrow { no, yes };
Comment on lines 1878 to +1880

@barcharcraz Charlie Barto (barcharcraz) Jan 20, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why must these be enum classes instead of simple bools?

It is fun to read nothrow NoThrow = nothrow::no (no, NoThrow!) but noexcept(IsNoThrow) might be clearer...

Pre-existing

@miscco Michael Schellenberger Costa (miscco) Jan 20, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Because it is much more difficult to reason about what meow<true> does compared to meow<nothrow::no>

Also it is much harder to accidentally pass the wrong thing

@miscco Michael Schellenberger Costa (miscco) Jan 20, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Especially in this case where it is

trace_iterator<Cat, true, false, true>

Are you sure those are in the right order?

@CaseyCarter Casey Carter (CaseyCarter) Jan 20, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

nothrow is a terrible name. How many times have I told someone not to put a negative in a name?

@miscco Michael Schellenberger Costa (miscco) Jan 20, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But you told them not to do it.

Nobody said you could not commit horrible code crimes

@barcharcraz Charlie Barto (barcharcraz) Jan 21, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

May I suggest something giving the best of both worlds (in the future, this is pre-existing, also using enum helps a ton):

enum : bool { Throws, NoThrow };
template<bool IsNoThrow = false>
struct meow { ... };

meow<NoThrow> ....

@barcharcraz Charlie Barto (barcharcraz) Jan 21, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suppose you could name the enum if you really want to, but that isn't relevant for clarity, just for preventing type confusion, but type confusion is arguably not a big deal here since really the type is bool for all of them.


static constexpr int sentinel_position = 42;

template <class Category, sized Sized = sized::no, assign Assign = assign::no>
template <class Category, sized Sized = sized::no, assign Assign = assign::no, nothrow NoThrow = nothrow::no>
struct trace_iterator {
using value_type = int;
using difference_type = int;
Expand All @@ -1891,21 +1892,23 @@ namespace iter_ops {
static constexpr bool is_sized = Sized == sized::yes;

trace_iterator() = default;
constexpr explicit trace_iterator(trace& t) : trace_{&t} {}
constexpr explicit trace_iterator(int const pos, trace& t) : trace_{&t}, pos_{pos} {}
constexpr explicit trace_iterator(trace& t) noexcept(NoThrow == nothrow::yes) : trace_{&t} {}
constexpr explicit trace_iterator(int const pos, trace& t) noexcept(NoThrow == nothrow::yes)
: trace_{&t}, pos_{pos} {}

trace_iterator(trace_iterator const&) requires is_forward = default;
trace_iterator(trace_iterator&&) = default;

constexpr trace_iterator& operator=(trace_iterator const& that) requires is_forward {
constexpr trace_iterator& operator=(trace_iterator const& that) noexcept(
NoThrow == nothrow::yes) requires is_forward {
if (!trace_) {
trace_ = that.trace_;
}
++trace_->assignments_;
pos_ = that.pos_;
return *this;
}
constexpr trace_iterator& operator=(trace_iterator&& that) {
constexpr trace_iterator& operator=(trace_iterator&& that) noexcept(NoThrow == nothrow::yes) {
if (!trace_) {
trace_ = that.trace_;
}
Expand All @@ -1915,65 +1918,71 @@ namespace iter_ops {
}

// clang-format off
constexpr trace_iterator& operator=(default_sentinel_t) requires (Assign == assign::yes) {
constexpr trace_iterator& operator=(default_sentinel_t) noexcept(NoThrow == nothrow::yes)
requires (Assign == assign::yes) {
++trace_->assignments_;
pos_ = sentinel_position;
return *this;
}
// clang-format on

int operator*() const;
int operator*() const noexcept(NoThrow == nothrow::yes);

constexpr trace_iterator& operator++() {
constexpr trace_iterator& operator++() noexcept(NoThrow == nothrow::yes) {
++trace_->increments_;
++pos_;
return *this;
}
trace_iterator operator++(int);
trace_iterator operator++(int) noexcept(NoThrow == nothrow::yes);

constexpr bool operator==(default_sentinel_t) const {
constexpr bool operator==(default_sentinel_t) const noexcept(NoThrow == nothrow::yes) {
++trace_->compares_;
return pos_ == sentinel_position;
}
constexpr int operator-(default_sentinel_t) const requires is_sized {
constexpr int operator-(default_sentinel_t) const noexcept(NoThrow == nothrow::yes) requires is_sized {
++trace_->differences_;
return pos_ - sentinel_position;
}
friend constexpr int operator-(default_sentinel_t, trace_iterator const& i) requires is_sized {
friend constexpr int operator-(default_sentinel_t, trace_iterator const& i) noexcept(
NoThrow == nothrow::yes) requires is_sized {
return -(i - default_sentinel);
}

constexpr bool operator==(trace_iterator const& that) const requires is_forward {
constexpr bool operator==(trace_iterator const& that) const
noexcept(NoThrow == nothrow::yes) requires is_forward {
++trace_->compares_;
return pos_ == that.pos_;
}

constexpr trace_iterator& operator--() requires is_bidi {
constexpr trace_iterator& operator--() noexcept(NoThrow == nothrow::yes) requires is_bidi {
++trace_->decrements_;
--pos_;
return *this;
}
trace_iterator operator--(int) requires is_bidi;
trace_iterator operator--(int) noexcept(NoThrow == nothrow::yes) requires is_bidi;

std::strong_ordering operator<=>(trace_iterator const&) const requires is_random;
std::strong_ordering operator<=>(trace_iterator const&) const
noexcept(NoThrow == nothrow::yes) requires is_random;

constexpr trace_iterator& operator+=(int const n) requires is_random {
constexpr trace_iterator& operator+=(int const n) noexcept(NoThrow == nothrow::yes) requires is_random {
++trace_->seeks_;
pos_ += n;
return *this;
}
trace_iterator operator+(int) const requires is_random;
friend trace_iterator operator+(int, trace_iterator const&) requires is_random {}
trace_iterator operator+(int) const noexcept(NoThrow == nothrow::yes) requires is_random;
friend trace_iterator operator+(int, trace_iterator const&) noexcept(
NoThrow == nothrow::yes) requires is_random {}

trace_iterator& operator-=(int) requires is_random;
trace_iterator operator-(int) const requires is_random;
trace_iterator& operator-=(int) noexcept(NoThrow == nothrow::yes) requires is_random;
trace_iterator operator-(int) const noexcept(NoThrow == nothrow::yes) requires is_random;

constexpr int operator-(trace_iterator const& that) const requires is_random || is_sized {
constexpr int operator-(trace_iterator const& that) const
noexcept(NoThrow == nothrow::yes) requires is_random || is_sized {
++trace_->differences_;
return pos_ - that.pos_;
}

int operator[](int) const;
int operator[](int) const noexcept(NoThrow == nothrow::yes);

trace* trace_ = nullptr;
int pos_ = 0;
Expand Down Expand Up @@ -2826,25 +2835,27 @@ namespace iter_ops {
}
STATIC_ASSERT(test_iter_count_sentinel_forms());

template <nothrow NoThrow = nothrow::no>
struct sized_test_range {
mutable trace t{};

constexpr unsigned char size() const {
constexpr unsigned char size() const noexcept(NoThrow == nothrow::yes) {
++t.sizes_;
return 42;
}
int* begin() const;
int* end() const;
int* begin() const noexcept(NoThrow == nothrow::yes);
int* end() const noexcept(NoThrow == nothrow::yes);
};

template <nothrow NoThrow = nothrow::no>
struct unsized_test_range {
mutable trace t{};

constexpr trace_iterator<forward_iterator_tag> begin() const {
constexpr trace_iterator<forward_iterator_tag> begin() const noexcept(NoThrow == nothrow::yes) {
++t.begins_;
return trace_iterator<forward_iterator_tag>{t};
}
constexpr std::default_sentinel_t end() const {
constexpr std::default_sentinel_t end() const noexcept(NoThrow == nothrow::yes) {
++t.ends_;
return {};
}
Expand All @@ -2857,46 +2868,107 @@ namespace iter_ops {
{
// Call distance(i, s) with: sized_sentinel_for<S, I> && input_iterator<I> && last - first > 0
// Validate return is last - first
using I = trace_iterator<input_iterator_tag, sized::yes>;
using I = trace_iterator<input_iterator_tag, sized::yes, assign::no, nothrow::no>;
trace t{};
auto const result = distance(I{t}, default_sentinel);
STATIC_ASSERT(same_as<decltype(result), iter_difference_t<I> const>);
I first{t};
same_as<iter_difference_t<I>> auto const result = distance(first, default_sentinel);
STATIC_ASSERT(!noexcept(distance(first, default_sentinel)));
assert(result == sentinel_position);
assert((t == trace{.differences_ = 1}));
}
{
// Call distance(i, s) with: sized_sentinel_for<S, I> && input_iterator<I> && last - first > 0
// Validate return is last - first
using I = trace_iterator<input_iterator_tag, sized::yes, assign::no, nothrow::yes>;
trace t{};
I first{t};
same_as<iter_difference_t<I>> auto const result = distance(first, default_sentinel);
STATIC_ASSERT(noexcept(distance(first, default_sentinel)));
assert(result == sentinel_position);
assert((t == trace{.differences_ = 1}));
}

{
// Call distance(i, s) with:
// sized_sentinel_for<S, I> && forward_iterator<I> && same_as<S, I> && last - first < 0
// Validate return is last - first
using I = trace_iterator<forward_iterator_tag, sized::yes>;
using I = trace_iterator<forward_iterator_tag, sized::yes, assign::no, nothrow::no>;
trace t{};
auto const result = distance(I{sentinel_position, t}, I{t});
STATIC_ASSERT(same_as<decltype(result), iter_difference_t<I> const>);
I first{sentinel_position, t};
I last{t};
same_as<iter_difference_t<I>> auto const result = distance(first, last);
STATIC_ASSERT(!noexcept(distance(first, last)));
assert(result == -sentinel_position);
assert((t == trace{.differences_ = 1}));
}
{
// Call distance(i, s) with:
// sized_sentinel_for<S, I> && forward_iterator<I> && same_as<S, I> && last - first < 0
// Validate return is last - first
using I = trace_iterator<forward_iterator_tag, sized::yes, assign::no, nothrow::yes>;
trace t{};
I first{sentinel_position, t};
I last{t};
same_as<iter_difference_t<I>> auto const result = distance(first, last);
STATIC_ASSERT(noexcept(distance(first, last)));
assert(result == -sentinel_position);
assert((t == trace{.differences_ = 1}));
}

{
// Call distance(i, s) with: !sized_sentinel_for<S, I> && input_iterator<I> && last - first > 0
// Validate return is last - first, and increment is called last - first times
using I = trace_iterator<input_iterator_tag>;
using I = trace_iterator<input_iterator_tag, sized::no, assign::no, nothrow::no>;
trace t{};
auto const result = distance(I{t}, default_sentinel);
STATIC_ASSERT(same_as<decltype(result), iter_difference_t<I> const>);
I first{t};
same_as<iter_difference_t<I>> auto const result = distance(move(first), default_sentinel);
STATIC_ASSERT(!noexcept(distance(move(first), default_sentinel)));
assert(result == sentinel_position);
assert((t == trace{.compares_ = sentinel_position + 1, .increments_ = sentinel_position}));
}
{
// Call distance(i, s) with: !sized_sentinel_for<S, I> && input_iterator<I> && last - first > 0
// Validate return is last - first, and increment is called last - first times
using I = trace_iterator<input_iterator_tag, sized::no, assign::no, nothrow::yes>;
trace t{};
I first{t};
same_as<iter_difference_t<I>> auto const result = distance(move(first), default_sentinel);
STATIC_ASSERT(!noexcept(distance(move(first), default_sentinel))); // No conditional noexcept
assert(result == sentinel_position);
assert((t == trace{.compares_ = sentinel_position + 1, .increments_ = sentinel_position}));
}

{
// Call distance(r) with: sized_range<R>, validate that begin and end are not called
sized_test_range<nothrow::no> r{};
same_as<ptrdiff_t> auto const result = distance(r);
STATIC_ASSERT(!noexcept(distance(r)));
assert(result == 42);
assert((r.t == trace{.sizes_ = 1}));
}
{
// Call distance(r) with: sized_range<R>, validate that begin and end are not called
sized_test_range r{};
auto const result = distance(r);
STATIC_ASSERT(same_as<decltype(result), std::ptrdiff_t const>);
sized_test_range<nothrow::yes> r{};
same_as<ptrdiff_t> auto const result = distance(r);
STATIC_ASSERT(noexcept(distance(r)));
assert(result == 42);
assert((r.t == trace{.sizes_ = 1}));
}

{
// Call distance(r) with: !sized_range<R>, validate that begin and end are called
unsized_test_range<nothrow::no> r{};
assert(distance(r) == sentinel_position);
STATIC_ASSERT(!noexcept(distance(r)));
trace const expected{
.compares_ = sentinel_position + 1, .increments_ = sentinel_position, .begins_ = 1, .ends_ = 1};
assert(r.t == expected);
}
{
// Call distance(r) with: !sized_range<R>, validate that begin and end are called
unsized_test_range r{};
unsized_test_range<nothrow::yes> r{};
assert(distance(r) == sentinel_position);
STATIC_ASSERT(!noexcept(distance(r))); // No conditional noexcept
trace const expected{
.compares_ = sentinel_position + 1, .increments_ = sentinel_position, .begins_ = 1, .ends_ = 1};
assert(r.t == expected);
Expand Down