6
\$\begingroup\$

This is a range class, similar in functionality to the range function in python. It was originally made to work with just range-based for loops, like so:

for(auto i : range(256))
    // do some code

but was workshopped by some people better at C++20 than me into being able to be used with C++20 ranges, like so:

auto is_even [](int x) { return x % 2 == 0; }
for(auto i : range(3, 500, 7) | std::views::filter(is_even))
    //do some code

The code:

#include <ranges>

template <std::integral IntType>
struct range : std::ranges::view_interface<range<IntType>> {
    using value_type = IntType;
    using difference_type = std::ptrdiff_t;

    range() : range(0) { }

    explicit range(value_type stop) : range(0, stop) { }

    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");
    }

    value_type operator*() const { return m_value; }

    range& operator++() {
        m_value += m_step;
        return *this;
    }
    range operator++(int) {
        auto old = *this;
        ++*this;
        return old;
    }

    friend bool operator==(range, range) = default;
    friend bool operator==(range r, std::default_sentinel_t) {
        if (r.m_step > 0) return r.m_value >= r.m_stop;
        else return r.m_value <= r.m_stop;
    }

    friend range begin(range r) { return r; }
    friend std::default_sentinel_t end(range r) { return {}; }

private:
    value_type m_value;
    value_type m_stop;
    value_type m_step;
};
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Hi. Welcome to Code Review! The "workshopped by some people better at C++20 than me" suggests that this isn't your code. Can you confirm that this is in fact code that you own and have the right to post under the CC license that Stack Exchange uses? \$\endgroup\$
    – mdfst13
    Commented Sep 18, 2021 at 5:59
  • \$\begingroup\$ Well we collaborated on it together. They changed my original code, and I changed the code after they added their code to it. So yes, I own it, alongside them. \$\endgroup\$
    – fortytoo
    Commented Sep 18, 2021 at 6:02

3 Answers 3

5
\$\begingroup\$

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.)
\$\endgroup\$
5
  • \$\begingroup\$ Would you recommend having the step being std::optional, with increment operator being used instead of a step when a step is not explicitly given? Or require start, stop and step to be given at construction by the user? That's all I can come up with as a solution to the "don't default step to 1" comment. \$\endgroup\$
    – fortytoo
    Commented Sep 19, 2021 at 4:29
  • \$\begingroup\$ Hm, good question. I think what I might do is make range() a function, and return different types depending on the number of arguments. The 0, 1, and 2 argument overloads could simply return a std::ranges::iota_view. The 3 argument would return a custom type that has a customizable step value, like your existing range class. Since you only get the custom step when you explicitly ask for it, there is no need to special-case a step size of ±1, or assume 1 as a default. \$\endgroup\$
    – indi
    Commented Sep 19, 2021 at 13:25
  • \$\begingroup\$ I really don't like std::ranges::iota_view with constructor of single element setting starting value instead of final value is absolutely horrendous design. No idea why it was approved this way in C++20. \$\endgroup\$
    – ALX23z
    Commented Sep 20, 2021 at 9:49
  • \$\begingroup\$ I've found that it can be handy to have the range view object act like an iterator (codeproject.com/Articles/1245954/…) but it is still a distinct type, and doing that is basically a shortcut. \$\endgroup\$
    – JDługosz
    Commented Sep 20, 2021 at 14:32
  • \$\begingroup\$ Eh, not convinced the “simplifications” are worth muddying the concept space so much. If you do it, then you can only use your hybrid “iteranger” types… can’t use standard ranges, or any other type that plays by the normal rules. There are better, generic ways to achieve the stuff you mentioned. Like, the off-by-one thing is just for (auto i : rv1 | drop(1))… which is is much clearer (or make a subrange and do for (auto i : rng.advance(1))). And find_something() could return something like optional<It> (or just T*), or {It, bool} and use structured bindings. \$\endgroup\$
    – indi
    Commented Sep 24, 2021 at 21:08
3
\$\begingroup\$

Unnecessary restriction of the value_type

You restrict the value_type of a range to the integral types, but I see no reason why that is necessary. It would work equally well with float and double, and any other type that you can add and compare with (consider for example std::chrono::duration).

This can be taken further to have different types for start, stop and step. For example, instead of integers, consider using an iterator for start and stop, and being able to do:

std::vector<std::string> names{...};

// Print every other name
for (auto &name: range(names.begin(), names.end(), 2))
   std::cout << *name << '\n';

This can be done like so:

template <typename StopType = int, typename StartType = StopType,
          typename StepType = decltype(StopType{} - StartType{})>
struct range: std::ranges::view_interface<range<StopType, StartType, StepType>> {
    range(): range(0) {}
    explicit range(StopType stop): range(StopType{}, stop) {}
    explicit range(StartType start, StopType stop,
                   StepType step = decltype(stop - start){1}):
        m_value{start}, m_stop{stop}, m_step{step}
    {
        if (step == StepType{})
            throw std::domain_error("Step cannot be zero");
    }
    ...
};

Then even the following would work:

std::vector<std::string> words{"Aap", "Noot", "Mies", "Wim", "Zus", "Jet"};
for (auto word: range(words.begin(), words.end(), 2))
    std::cout << *word << "\n";

for (auto t: range(std::chrono::minutes(10)))
    std::cout << std::chrono::duration_cast<std::chrono::seconds>(t).count() << '\n';

Make begin() and end() member functions

The functions begin() and end() should be member functions, not friend functions. If they are member functions, std::begin(range(...)) will still work, but if it is a friend, then range(...).begin() will fail to compile.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Good answer. But “It would work equally well with float and double” is not completely true. Floating-point step size tends to produce surprising results. You’d have to do some work ahead of time to produce the sequence that the user expects to get. \$\endgroup\$ Commented Sep 18, 2021 at 22:51
1
\$\begingroup\$

range is not good choice for name for a class in C++. It somewhat conflicts with std::ranges::range concept - even when objects are in different namespaces you should avoid using the same names especially for frequently used classes. Consider index_range.

It is a good replacement for std::ranges::iota_view which I find to be terribly designed because std::ranges::iota_view(10) makes it start from 10 instead of finishing with 10... which makes it unsuitable for the simple for-loop.

Design:

You should make begin()/end() return identical iterators. Since it is possible - you should. The thing is, frequently iterator based algorithms require both iterators to be identical and currently ranges library doesn't seem to have much multi-threaded algorithms. o if you had a function that accepts a range but calls such a function that relies on iterators you'd get an error with this class. It has uses outside of for-loop - you should take that into account during your design.

I am not 100% certain but I believe you should separate the class into two cases: when step = 1 and for arbitrary case. The thing is that step = 1 is the most common case by far and compilers should be able to optimize it much easier than those with arbitrary steps - so you'll hopefully reach performance of a typical C-style for-loop. You can achieve that by making index_range being a function that returns a class and just return different class when number of arguments is 3.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.