ericniebler / range-v3

Range library for C++14/17/20, basis for C++20's std::ranges
Other
4.14k stars 439 forks source link

Zip view can create surprise copies when not desired #1346

Closed FunMiles closed 5 years ago

FunMiles commented 5 years ago

I have something like this:

class Combo {
public:
    Combo();
    auto asRange() const { return ranges::views::zip( x, y ); }
private:
    X x;
    Y y;
};

If the copy constructor of Y is not deleted, then the result zipped object makes a copy of y. If it is deleted, then it will use a reference. This behavior is very surprising. I discovered it because I am using the range in a way that copies pointers and if there's a copy, the pointer will be invalid after the destruction of the zipped range.

I think in all cases, the zip view should not make a copy. It is called a view after all. Is this a bug in the implementation or the behavior for which it is designed? If the latter, I suggest changing the design as it sounds counter to the naming.

ericniebler commented 5 years ago

It depends on what X and Y are. If they are containers, they are not copied. If they are views, the assumption is that it is lightweight and non-owning, so a copy is made to avoid lifetime issues.

What are X and Y in your case?

FunMiles commented 5 years ago

It depends on what X and Y are. If they are containers, they are not copied. If they are views, the assumption is that it is lightweight and non-owning, so a copy is made to avoid lifetime issues.

What are X and Y in your case?

They are containers. I have the following more complete example:

#include <iostream>
#include <range/v3/all.hpp>

class X {
public:
    auto begin() const { return v.begin(); }
    auto end() const { return v.end(); }
private:
    std::vector<double> v;

};

class Y {
public:
public:
    Y() {}
    Y(const Y &y) : v(y.v){
        std::cout << "Copied y" << std::endl;
    }

    auto begin() const { return v.begin(); }
    auto end() const { return v.end(); }
private:
    std::vector<int> v;
};

class Combo {
public:
    Combo() {}
    auto asRange() const { return ranges::views::zip( x, y ); }
private:
    X x;
    Y y;
};

int main() {
    Combo cmb;
    const auto r = cmb.asRange();
    return 0;
}

The output shows that at the cmb.asRange() line, Y is copied 3 times!

I am using Mac OS catalina with the latest clang++ compiler from XCode and the range library installed from brew.

ericniebler commented 5 years ago

You are using the range libraries heuristics for guessing what is a view and what is a container. If you also provide non-const overloads of begin and end that return mutable iterators, the huristic will guess correctly. If that is wrong for your use case, you need to specialize ranges::enable_view to tell the range library these are containers, not views.

Edit: see https://en.cppreference.com/w/cpp/ranges/view for more information.

FunMiles commented 5 years ago

Eric, I like the range library. Thanks for your work. Is the final version with concept going to avoid heuristics? A library where you get caught by surprise like this can be very discouraging and make people turn away. I am not saying heuristics are bad. But I remember the saying 'make the library easy to use and hard to misuse'. I feel that I fell into a trap of easy to misuse.

PS: My objects are supposed to be immutable, hence only the const version. Having to put a non-const begin/end pair would not be good. Also, instead of having to specialize ranges::enable_view to tell the range library that this is not a view, shouldn't the views inherit from a common parent to say they are a view, thus solving the issue more cleanly, IMHO? (The link you send says they should inherit from view_base)

PPS: Thanks for the link to the cppreference for ranges. I didn't know it existed. I will look into there from now on for more info.