Design review
This is obviously a useful type. Python coders swear by their version of it, and it would be wonderful to be able to write simple numeric loops with safe, clear range-for
constructs, rather than clunky and dangerous old-school for
loops.
But there are a lot of problems with your current design. Let’s start with the name.
“Range” already means something in C++, and it doesn’t mean this. It is bad practice to simply lift something from one language and dump it into another. C++ and Python are entirely different beasts, without even much of a common heritage to speak of. All you’re going to accomplish by doing this is confusing C++ coders. In C++, we call this operation iota
.
As if that weren’t already confusing enough… is this a range (or a view)? Or… is it an iterator? It is some kind of bastardized chimera of the two.
Look at the kind of gibberish you can write with the current class:
auto rng = range{69}; // no problem, create the range
auto it = std::ranges::begin(rng); // no problem, get your start iterator
rng == it; // whut? the range equals the start iterator?
++rng; // whut? incrementing the range (not the iterator)
rng == ++it; // the hell? O_o
The problem here is conceptual confusion. A range is a range, and an iterator is an iterator. You have created a thing that is both… sorta. It’s kind of a range; you can construct it, then iterate over it. But… it’s also what you iterate with. I guess it… kinda… looks like a forward iterator? But it’s not.
If you look at std::ranges::views::iota<W, Bound>
, it has a custom iterator type. The iterator type is hidden—it’s an implementation detail—but the point is that it’s an honest-to-goodness iterator, and it is not the same type as std::ranges::views::iota<W, Bound>
. It can be anything from an input iterator to a random access iterator, depending on the capabilities of W
.
By making the range type also the iterator type, you have created a bit of a mess that may appear to work for simple cases (like for (auto i : range{10})
), but will probably fail in bizarre ways for more complex cases.
Try this, for example:
auto is_even = [](auto x) { return x % 2 == 0; };
// works fine, prints: 8 6 4 2
for (int i : std::ranges::iota_view{1, 10} | std::views::filter(is_even) | std::views::reverse)
std::cout << i << ' ';
std::cout << '\n';
// works until you try to uncomment the last bit
for(auto i : range{1, 10} | std::views::filter(is_even) /* | std::views::reverse */ )
std::cout << i << ' ';
Here’s another design aspect that is missing: in your view, you store the current value, the stop value, and the step value… but you don’t store the start value. As a result:
auto one_to_ten = range{1, 10};
std::cout << *(std::ranges::begin(one_to_ten)); // prints 1, as expected
++one_to_ten;
std::cout << *(std::ranges::begin(one_to_ten)); // prints 2 ???
This is another symptom of the conceptual confusion. Start, stop, and step are properties of the view… but the current value is a property of the iterator.
I think you need to completely rethink the design. You need a range/view… and you need an iterator… and those are not the same thing. I suggest studying the design of std::ranges::iota_view
for inspiration. Your type is basically std::ranges::iota_view
, except that it also has a customizable step. That’s a worthwhile improvement, but you should probably base your design on the standard type.
Code review
template <std::integral IntType>
G. Sliepen already explained how you could relieve this restriction. I’d suggest taking a cue from std::ranges::iota_view
, and making the concept std::weakly_incrementable
.
using value_type = IntType;
using difference_type = std::ptrdiff_t;
Do you really need these? Particularly difference_type
seems superfluous. Unless you need something for the interface, you shouldn’t add it. It just creates an unnecessary maintenance burden.
range() : range(0) { }
Good on you for using constructor delegation rather than a default argument, as far too many C++ coders would do. (Of course, you use a default argument in another constructor, which is bad, but we’ll get to that later.)
However, there are a lot of things you can do to improve this.
First, using a literal 0
will probably be fine if you’re restricting the type to integers, but if you’re going to expand support, this is going to be a problem. Rather than using literal 0
, you should use default initialization:
range() : range{IntType{}} {}
explicit range(IntType stop) : range{IntType{}, std::move(stop)} {}
range(IntType start, IntType stop) : range{std::move(start), std::move(stop), IntType(1)} {}
range(IntType start, IntType stop, IntType step)
: m_value{std::move(start)}
, m_stop{std::move(stop)}
, m_step{std::move{step}}
{}
The tricky part will be handling the default step value, because literal 1
won’t be a good default for many types. That’s something you’ll have to figure out.
You should also consider constexpr
for all these constructors.
range(value_type start, value_type stop, value_type step = 1) :
m_value{ start }, m_stop{ stop }, m_step{ step }
{
if (step == 0) throw std::domain_error("step cannot be zero");
}
As I mentioned above, you should use constructor delegation, not default arguments. Default arguments cause endless headaches.
Also, you check whether step
is zero… but what if it’s negative? Is that okay? (Seems so, from the sentinel condition.) If so, shouldn’t you also check that start
is greater than or equal to stop
? In any case, assuming a positive step, why not check that start
is less than or equal to stop
?
value_type operator*() const { return m_value; }
range& operator++() {
m_value += m_step;
return *this;
}
range operator++(int) {
auto old = *this;
++*this;
return old;
}
Alright, here’s where things really get messy. These are not view operations, these are iterator operations. For a view, all you need is begin()
and end()
. std::view_interface
is smart enough to check the view’s iterator type, and deduce the properties of the view from there. You would automatically get empty()
, size()
, front()
, and operator[]
, and others, if possible.
Assuming these functions were in an iterator type, there are still dragons here that you need to be aware of.
Let’s say IntType
is a 16-bit unsigned type, so the values range from 0 to 65,535. Let’s assume someone did range{0, 50'000, 40'000}
. Now you’d expect this to give you 0, 40,000, and then that’s it, right? What you’re going to get is: 0, 40,000, 14,464. Why? Because you are not taking overflow into account.
This is why std::ranges::iota_view
doesn’t have a customizable step. It opens to the door to a whole host of complications.
friend range begin(range r) { return r; }
This is the clearest sign of the problem with your design. Can you imagine if std::vector<T>::begin()
returned a copy of the vector?
Final notes
This is not a bad idea: a version of std::ranges::iota_view
that has a variable step could be useful.
There are a few hurdles you have to overcome, though.
- You need to figure out a good default for the step.
1
works for integer types, but it won’t work more generally. (You could just give up on being completely general, of course.)
- You need to do something about overflow/underflow when calculating. This is not a problem for
std::ranges::iota_view
, because it just does ++
and --
, and stops when the value equals the start or stop; it can’t possibly overshoot. Since you’re incrementing in steps possibly greater than a single step, you could hop over the start/stop, and that’s where you could get problems. (Again, this becomes much more complicated if you want to support more than just integers.)