Re: [Boost-users] Reverse for loop with boost::adaptors::reverse crashes

Subject: Re: [Boost-users] Reverse for loop with boost::adaptors::reverse crashes
From: Gavin Lambert (gavinl_at_[hidden])
Date: 2014-11-27 19:00:26

On 26/11/2014 05:11, Filip Konvička wrote:
> I was trying to replace BOOST_REVERSE_FOREACH with the standard
> range-based for loop.
>
> #include <boost/range/adaptor/reversed.hpp>
> #include <boost/foreach.hpp>
> #include <vector>
> #include <iostream>
>
> std::vector<int> getv() {
>    return std::vector<int>{1,2,3};
> }
>
> int main(int argc, char* argv[]) {
>    // works fine:
>    BOOST_REVERSE_FOREACH(int i, getv())
>      std::cout << i << std::endl;
>    // crashes:
>    for(int i : boost::adaptors::reverse(getv()))
>      std::cout << i << std::endl;
>    return 0;
> }
>
> Maybe this is basic C++ I should understand; it looks like the temporary
> vector returned by getv() gets destroyed in the 2nd loop too soon.  I
> was wondering whether the adaptor could warn me that I'm using it in a
> wrong way (the code looks so nice and compiles cleanly...).

This works (albeit possibly with a performance penalty in the form of a 
double copy; I'm not sure if the compiler can be smart enough to elide 
that):

template<typename T, typename R>
std::vector<T> as_vector(const R& range)
{
     return std::vector<T>(boost::begin(range), boost::end(range));
}

...

     for (int i : as_vector<int>(boost::adapters::reverse(getv())))

The original code is unsafe because the compiler has no way to know that 
the temporary vector returned by getv() needs to live on, as normally 
that is not the case with function arguments.  (And the range adapters 
do not make a copy of their arguments, for performance reasons, so they 
require a longer lifetime.)

But at least in theory the range-based-for itself has to preserve the 
"final" temporary range for the duration of the loop, as otherwise it is 
simply not safe to ever use an rvalue as the range expression.  (I'm not 
sure if this is mandated by the standard, but it seemed to work in 
VS2013 at least.)  And a collection is itself a valid range, so ensuring 
that you iterate over a "real" collection instead of just an adapter works.

It seems likely that a method like as_vector already exists somewhere in 
Boost, although I can't put my finger on it at the moment.  There's also 
probably a way to make it deduce T from R so it doesn't have to be 
explicitly specified, but I'm not really familiar enough with 
Boost.Range to write that.  (In general I find the Boost.Range 
documentation fairly incomprehensible.)

It might be easier just to rewrite it like this though, which will 
satisfy the lifetime requirement while avoiding the possible performance 
penalty:

     auto v = getv();
     for (int i : boost::adapters::reverse(v)) {...}