ericniebler / range-v3

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

view::transform(&T::data_member) of range-of-prvalues creates dangling references, guaranteed #951

Open voivoid opened 5 years ago

voivoid commented 5 years ago

Hi!

The following toy example produces undefined result on latest gcc and clang with -O2 ( by the way results with -O0 are fine ) with range-v3 master branch.

#include "range/v3/view/transform.hpp"
#include <iostream>
#include <vector>

struct State {
    size_t x;
};

int main() {

    std::vector<State> v = { {1}, {2}, {3} };
    auto r = v | ranges::view::transform( [](const State& o) { return o; } ) | ranges::view::transform( &State::x );

    for (const auto& x : r)
    {
        std::cout << x << std::endl;
    }

    return 0;
}

I guess the problem is that the result of the last transform is a reference to a temporary. If it's correct I wonder if some enchantment ( static_asserts maybe? ) could be made to prevent this?

Thanks!

ericniebler commented 5 years ago

In this very limited case, the transform view could have detected that (a) it was adapting a range of prvalues, and (b) the transformation function was a pointer-to-member that, when evaluated on a prvalue, would result in a reference into the temporary.

In the more general case, it is impossible for the transform view to know whether the references it returns are dangling or not. I'm not convinced it is worth adding a check, but I'll leave this issue open and think about it.

I might accept a PR if you were so motivated.

tower120 commented 5 years ago

@voivoid FYI, for case, when work with proxy object (freshly created one during iteration) is your intention - you can cache that object(this will increase view size on size(T), alternatively you can cache in unique_ptr):

https://wandbox.org/permlink/W8yAd0sQNGYl0NHv

#include "range/v3/view/transform.hpp"
#include <iostream>
#include <vector>

struct State {
    size_t x;
};

template<class T>
struct Cache{
    mutable std::optional<T> value;
    T& operator()(T&& o) const {
        value = std::move(o);
        return value.value();
    }
};

template<class T>
const auto cache_proxy = ranges::view::transform(Cache<T>{});

int main() {

    std::vector<State> v = { {1}, {2}, {3} };
    auto r = v 
        | ranges::view::transform( [](const State& o) { return o; } )
        | cache_proxy<State>        // putting a little more work - it's possible to auto-deduct State
        | ranges::view::transform( &State::x );

    for (const auto& x : r)
    {
        std::cout << x << std::endl;
    }

    return 0;
}