We were unable to load Disqus. If you are a moderator please see our troubleshooting guide.

Zarnivoop • 4 years ago

Ok, I gave it a try with maybe the simplest possible container which can only contain a sequence of non-negative integers. As far as I can tell, the iterator has all the bells and whistles mentioned in the article. But still, it does not compile. Range-for works, though. What could be the problem? Here is the implementation and a test bench.



#include <vector>
#include <iostream>
#include <string>
#include <ranges>


class Sequence
{
public:
using value_type = const std::size_t;
using size_type = std::size_t;

Sequence(std::size_t sz = 0) : N(sz) {}

std::size_t operator[](std::size_t i) const noexcept
{
return i;
}

bool empty() const noexcept
{
return !bool(N);
}

size_type size() const noexcept
{
return N;
}

class Iterator {
public:
typedef std::bidirectional_iterator_tag iterator_category;
typedef const std::size_t value_type;
typedef std::ptrdiff_t difference_type;
typedef value_type* pointer;
typedef value_type& reference;


Iterator() : _n(0), _maxN(0) {}
Iterator(std::size_t n, std::size_t mx) : _n(n), _maxN(mx) {}
Iterator(const Iterator&) = default;
Iterator(Iterator&&) = default;

reference operator*() const
{
return _n;
}

Iterator& operator++()
{
if (_n < _maxN)
++_n;
return *this;
}

Iterator operator++(int)
{
Iterator temp = *this;
++*this;
return temp;
}

Iterator& operator--()
{
if (_n > 0)
--_n;
return *this;
}

Iterator operator--(int)
{
Iterator temp = *this;
--*this;
return temp;
}

bool operator== (const Iterator& it) const
{
return _n == it._n;
}

bool operator!= (const Iterator& it) const
{
return _n != it._n;
}

private:
std::size_t _n = 0;
std::size_t _maxN = 0;
}; /* class Iterator */

Iterator begin() const
{
return Iterator(0, N);
}

Iterator end() const
{
return Iterator(N, N);
}

private:
std::size_t N;
}; /* class Sequence */


int main()
{
constexpr int N = 10;

/* 1. Range-for works for Sequence */
std::cout << "*** Range-for for Sequence ***\n";
for (auto i : Sequence{N})
std::cout << i << ' ';
std::cout << "\n";

/* 2. Ranges work for std::vector */
std::cout << "*** Ranges for std::vector ***\n";
std::vector<int> ints{0, 1, 2, 3, 4, 5};
auto even = [](int i) { return 0 == i % 2; };
auto square = [](int i) { return i * i; };

for (int i : ints | std::views::filter(even) | std::views::transform(square)) {
std::cout << i << ' ';
}
std::cout << "\n";

/* 3. Ranges do not work for Sequence */
#if 1 /* Set to zero and it compiles. */

std::cout << "*** Ranges for Sequence ***\n";
for (int i : Sequence{N} | std::views::filter(even) | std::views::transform(square)) {
std::cout << i << ' ';
}
std::cout << "\n";
#endif
}

/* g++ version 10.2.0
g++ range-iterator-test.cc -std=c++2a */

Nathan Reed • 4 years ago

Hi Zarnivoop,

If you put in this static assert:


static_assert(std::bidirectional_iterator<Sequence::Iterator>);

then it fails and gcc will print some diagnostics as to why:


required for the satisfaction of 'assignable_from<_Tp&, _Tp>' [with _Tp = Sequence::Iterator]
required for the satisfaction of 'movable<_Iter>' [with _Iter = Sequence::Iterator]
required for the satisfaction of 'weakly_incrementable<_Iter>' [with _Iter = Sequence::Iterator]
required for the satisfaction of 'input_or_output_iterator<_Iter>' [with _Iter = Sequence::Iterator]
required for the satisfaction of 'input_iterator<_Iter>' [with _Iter = Sequence::Iterator]
required for the satisfaction of 'forward_iterator<_Iter>' [with _Iter = Sequence::Iterator]
in requirements with '_Lhs __lhs', '_Rhs&& __rhs' [with _Rhs = Sequence::Iterator; _Lhs = Sequence::Iterator&]
note: the required expression '__lhs = static_cast<_Rhs&&>(__rhs)' is invalid
129 | { __lhs = static_cast<_Rhs&&>(__rhs) } -> same_as<_Lhs>;
| ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~

In short, an iterator has to be movable and in this case it failed to be move-assignable (couldn't assign an rhs of Iterator&& to an lhs of Iterator&.) This is because you explicitly defined the copy and move constructors of Iterator—even though they are defaulted, it prevents the compiler from auto-generating the copy and move assignment operators! Removing those definitions lets the compiler auto-generate everything, as normal. After this, both bidirectional_iterator and bidirectional_range are satisfied.

The code then fails to compile due to this other error:


note: candidate: 'constexpr auto std::ranges::views::__adaptor::operator|(_Range&&, const std::ranges::views::__adaptor::_RangeAdaptorClosure<_Callable>&) [with _Range = Sequence; _Callable = std::ranges::views::__adaptor::_RangeAdaptor<_Callable>::operator()<{main()::<lambda(int)>&}>::<lambda(_Range&&)>]'
1173 | operator|(_Range&& __r, const _RangeAdaptorClosure& __o)
| ^~~~~~~~
note: constraints not satisfied
In instantiation of 'constexpr auto std::ranges::views::__adaptor::operator|(_Range&&, const std::ranges::views::__adaptor::_RangeAdaptorClosure<_Callable>&) [with _Range = Sequence; _Callable = std::ranges::views::__adaptor::_RangeAdaptor<_Callable>::operator()<{main()::<lambda(int)>&}>::<lambda(_Range&&)>]':
required for the satisfaction of 'viewable_range<_Range>' [with _Range = Sequence]
note: no operand of the disjunction is satisfied
79 | && (borrowed_range<_Tp> || view<remove_cvref_t<_Tp>>);
| ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This is happening because the Sequence{N} object is a temporary. You can't directly iterate over views of a temporary in general, because the views don't take ownership of it and thus the temporary object doesn't last long enough—its lifetime is over as soon as that expression finishes evaluating, before you even get into the loop body.

If you store the Sequence object in a variable, then it works:


Sequence s{N};
for (int i : s | std::views::filter... )
{
...
}

Alternatively, if you make it a "borrowed range", that tells the ranges library that the iterators of this object can survive beyond the lifetime of the sequence object itself, and then it works also. That's done by specializing std::ranges::enable_borrowed_range to true:


template <>
inline constexpr bool std::ranges::enable_borrowed_range<Sequence> = true;

Hope that clears it up!

Zarnivoop • 4 years ago

Hi Nathan,

Thanks a lot for this. I'd never have guessed that defaulting something un-defaults something else. Wow.
I like the static_assert trick to make compiler give more intelligible
error messages. Maybe Concepts are from now on a bit less scary :)

Daniel Gakwaya • 4 years ago

Giving this a try on some custom class that wraps a dynamic array,simulating std::vector, with an iterator class as shown below , but the std::ranges::sort algorithm is failing with the error :

error C7602: 'std::ranges::_Sort_fn::operator ()': the associated constraints are not satisfied C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30037\include\algorithm(7594): note: see declaration of 'std::ranges::_Sort_fn::operator ()'

with msvc from visual studio 16.10 .

Tried to parse the reason for the problem with the possible implementation of std::ranges::sort from cppreference with no luck. Could you lend an eye on this ?

My iterator is shown below :




template <typename t="">
class BoxContainer
{
public :
class Iterator
{
public :
using iterator_category = std::random_access_iterator_tag;
using difference_type = std::ptrdiff_t;
using value_type = T;
using pointer_type = T*;
using reference_type = T&;

Iterator() = default;
Iterator(pointer_type ptr) : m_ptr(ptr) {}

reference_type operator*() const {
return *m_ptr;
}

pointer_type operator->() {
return m_ptr;
}

Iterator& operator++() {
m_ptr++; return *this;
}

Iterator operator++(int) {
Iterator tmp = *this;
++(*this);
return tmp;
}

Iterator& operator--() {
m_ptr--; return *this;
}

Iterator operator--(int) {
Iterator tmp = *this;
--(*this);
return tmp;
}


Iterator& operator+=(const difference_type offset) {
m_ptr += offset;
return *this;
}

Iterator operator+(const difference_type offset) const {
Iterator tmp = *this;
return tmp += offset;
}

Iterator& operator-=(const difference_type offset) {
return *this += -offset;
}

Iterator operator-(const difference_type offset) const {
Iterator tmp = *this;
return tmp -= offset;
}

difference_type operator-(const Iterator& right) const {
return m_ptr - right.m_ptr;
}

reference_type operator[](const difference_type offset) const {
return *(*this + offset);
}

bool operator<(const Iterator& right) const {
return m_ptr < right.m_ptr;
}


bool operator>(const Iterator& right) const {
return right < *this;
}


bool operator<=(const Iterator& right) const {
return !(right < *this);
}

bool operator>=(const Iterator& right) const {
return !(*this < right);
}

friend bool operator== (const Iterator& a, const Iterator& b) {
return a.m_ptr == b.m_ptr;
};


friend bool operator!= (const Iterator& a, const Iterator& b) {
return a.m_ptr != b.m_ptr;
};

private:
pointer_type m_ptr;
};

Iterator begin() { return Iterator(&m_items[0]); }
Iterator end() { return Iterator(&m_items[m_size]); }
}

private :
T * m_items;
size_t m_capacity;
size_t m_size;

};

int main(int argc, char **argv)
{
BoxContainer<int> data;
data.add(1);
data.add(2);
data.add(3);


std::ranges::sort(data,std::less<>{}); // Compiler error

return 0;
}


The full class template is available (in much more readable format) here : https://github.com/rutura/c...