Reverse for loop with boost::adaptors::reverse crashes
Hi, 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...). Thanks, Filip
On 25 November 2014 at 16:11, Filip Konvička <filip.konvicka@logis.cz> wrote:
Hi,
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...).
Hi, I maintain the Boost.Range library and this issue is the most frustrating one. I've been unable to come up with a solution that does not unacceptably deteriorate performance in valid cases. I've thrown the problem wide open to the list and a number of other people and there haven't been any solutions that wouldn't ruin the interface or performance. I can only apologise for failing to address this. I've tried for many hours to find a solution, but failed. Our hope is with Range V3 and the standardisation proposal from Eric Niebler. He has defined the lifetime of new Iterables and Range Concepts to avoid this problem. With a new interface we can fix the problem. I'm still open to solutions for the V2 Boost.Range. I'd certainly not like to be irrationally blocking progress. The current recommendation is to take a temporary outside of the loop. This can often be a const reference temporary to extend the lifetime sufficiently.
Thanks, Filip
Regards, Neil Groves
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...).
Hi, I maintain the Boost.Range library and this issue is the most frustrating one. I've been unable to come up with a solution that does not unacceptably deteriorate performance in valid cases. I've thrown the problem wide open to the list and a number of other people and there haven't been any solutions that wouldn't ruin the interface or performance. I can only apologise for failing to address this. I've tried for many hours to find a solution, but failed.
Our hope is with Range V3 and the standardisation proposal from Eric Niebler. He has defined the lifetime of new Iterables and Range Concepts to avoid this problem. With a new interface we can fix the problem.
I'm still open to solutions for the V2 Boost.Range. I'd certainly not like to be irrationally blocking progress. The current recommendation is to take a temporary outside of the loop. This can often be a const reference temporary to extend the lifetime sufficiently.
Regards, Neil Groves
Neil, Thanks for your explanation. There is absolutely no need to apologize. I believe this is rather a language design flaw than anything else. I think that it would be great if there was a section on this subject in the documentation (just from the user's perspective, no detailed analysis). I admit that I haven't read all of the Boost.Range documentation so I hope that I haven't missed it by accident. Thank you, Filip
On Tue, 25 Nov 2014 16:41:02 +0000 Neil Groves <neil@grovescomputing.com> wrote:
On 25 November 2014 at 16:11, Filip Konvička <filip.konvicka@logis.cz> wrote:
Hi,
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...).
Hi, I maintain the Boost.Range library and this issue is the most frustrating one. I've been unable to come up with a solution that does not unacceptably deteriorate performance in valid cases. I've thrown the problem wide open to the list and a number of other people and there haven't been any solutions that wouldn't ruin the interface or performance. I can only apologise for failing to address this. I've tried for many hours to find a solution, but failed.
Our hope is with Range V3 and the standardisation proposal from Eric Niebler. He has defined the lifetime of new Iterables and Range Concepts to avoid this problem. With a new interface we can fix the problem.
I'm still open to solutions for the V2 Boost.Range. I'd certainly not like to be irrationally blocking progress. The current recommendation is to take a temporary outside of the loop. This can often be a const reference temporary to extend the lifetime sufficiently.
Hi, Is this related to the issue I reported here? https://svn.boost.org/trac/boost/ticket/10789 Matei
On Tue, 25 Nov 2014 13:55:15 -0500 Matei David <matei@cs.toronto.edu> wrote:
On Tue, 25 Nov 2014 16:41:02 +0000 Neil Groves <neil@grovescomputing.com> wrote:
On 25 November 2014 at 16:11, Filip Konvička <filip.konvicka@logis.cz> wrote:
Hi,
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...).
Hi, I maintain the Boost.Range library and this issue is the most frustrating one. I've been unable to come up with a solution that does not unacceptably deteriorate performance in valid cases. I've thrown the problem wide open to the list and a number of other people and there haven't been any solutions that wouldn't ruin the interface or performance. I can only apologise for failing to address this. I've tried for many hours to find a solution, but failed.
Our hope is with Range V3 and the standardisation proposal from Eric Niebler. He has defined the lifetime of new Iterables and Range Concepts to avoid this problem. With a new interface we can fix the problem.
I'm still open to solutions for the V2 Boost.Range. I'd certainly not like to be irrationally blocking progress. The current recommendation is to take a temporary outside of the loop. This can often be a const reference temporary to extend the lifetime sufficiently.
Hi,
Is this related to the issue I reported here? https://svn.boost.org/trac/boost/ticket/10789
Looking more into this issue, I found this bug report from 2 years ago that describes what I think is the underlying problem: https://svn.boost.org/trac/boost/ticket/7630 Given that the issue (bug?) involves a use case as simple as the one described by OP, many people might be bothered by it. Not everyone will know to ask on this list. Worse still, some might invest time in uselessly reporting the same issue all over again. For these reasons, I strongly agree with OP in suggesting that a *big warning sign* should be included in the Boost Range 2.0 documentation about this issue: "Thou shalt not use boost range adaptors on rvalue containers in C++11 range-based for loops." If the note is already there, then it is not prominent enough. Regarding solutions or workarounds, the last post by neilgroves in the bug report is from 9 months ago, suggesting he has a solution. Are there any updates on that?
Is this related to the issue I reported here? https://svn.boost.org/trac/boost/ticket/10789
Looking more into this issue, I found this bug report from 2 years ago that describes what I think is the underlying problem: https://svn.boost.org/trac/boost/ticket/7630
Given that the issue (bug?) involves a use case as simple as the one described by OP, many people might be bothered by it. Not everyone will know to ask on this list. Worse still, some might invest time in uselessly reporting the same issue all over again. For these reasons, I strongly agree with OP in suggesting that a *big warning sign* should be included in the Boost Range 2.0 documentation about this issue:
"Thou shalt not use boost range adaptors on rvalue containers in C++11 range-based for loops."
If the note is already there, then it is not prominent enough.
Regarding solutions or workarounds, the last post by neilgroves in the bug report is from 9 months ago, suggesting he has a solution. Are there any updates on that?
Thanks for the links. As Neil said earlier in this thread, there is currently no good solution; I guess we've got to live with that. To be honest, I still think that the design of the range-based for loop is strange. This problem applies also to other use cases than those involving Boost.Range. I understand why the temporaries get destroyed, but is there a rationale for that from the language design perspective? Isn't it more intuitive to let the temporaries live until the loop finishes? Maybe this is a question that I should ask somewhere else. By the way, does anyone have a link to Eric's proposal? Thanks, Filip
On Tue, Nov 25, 2014 at 5:40 PM, Filip Konvička <filip.konvicka@logis.cz> wrote:
Is this related to the issue I reported here?
Looking more into this issue, I found this bug report from 2 years ago that describes what I think is the underlying problem: https://svn.boost.org/trac/boost/ticket/7630
Given that the issue (bug?) involves a use case as simple as the one described by OP, many people might be bothered by it. Not everyone will know to ask on this list. Worse still, some might invest time in uselessly reporting the same issue all over again. For these reasons, I strongly agree with OP in suggesting that a *big warning sign* should be included in the Boost Range 2.0 documentation about this issue:
"Thou shalt not use boost range adaptors on rvalue containers in C++11 range-based for loops."
If the note is already there, then it is not prominent enough.
Regarding solutions or workarounds, the last post by neilgroves in the bug report is from 9 months ago, suggesting he has a solution. Are there any updates on that?
Thanks for the links. As Neil said earlier in this thread, there is currently no good solution; I guess we've got to live with that.
To be honest, I still think that the design of the range-based for loop is strange. This problem applies also to other use cases than those involving Boost.Range. I understand why the temporaries get destroyed, but is there a rationale for that from the language design perspective? Isn't it more intuitive to let the temporaries live until the loop finishes? Maybe this is a question that I should ask somewhere else.
By the way, does anyone have a link to Eric's proposal?
Thanks, Filip
I think [ http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4128.html ] is the latest proposal. Lee
I think [ http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4128.html ] is the latest proposal.
Thanks. I think that in the document section 3.3.2 nicely illustrates the issue, and section 14.1 describes a way to tell whether the argument is an 'Iterable'. So isn't there a way to disable the misuse with the currently available weapons? Isn't it possible to test that a value to be converted to a range is an r-value non-Range? Thanks, Filip
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)) {...}
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())))
Thanks, this might be helpful. I am using the following when I write functions that return a range of any_iterators to something that is a temporary vector: namespace detail { template<typename PBackupVector, typename I> struct it_PXYZ_Backup2 : public I { /// A shared pointer to the backup container. /// The backup container is deleted with the last iterator to it. PBackupVector pBK; /// Construction - increment use count of the container, /// and copy the real iterator. it_PXYZ_Backup2(PBackupVector const& pBK, I const& it) : I(it) , pBK(pBK) {} }; } /// Copies the container contents to a temporary vector and returns /// iterator range over the copy. /// The temporary vector is automatically removed when the last /// iterator is destroyed. template<typename it_PXYZ, typename C> iterator_range<it_PXYZ> iterators_from_copy(C const& container) { typedef std::vector<typename C::value_type> BackupVector; typedef shared_ptr<BackupVector> PBackupVector; PBackupVector pBackupVector = PBackupVector(new BackupVector(container.begin(), container.end())); typedef detail::it_PXYZ_Backup2< PBackupVector, typename BackupVector::const_iterator> it; return boost::make_iterator_range( it_PXYZ(it(pBackupVector, pBackupVector->begin())), it_PXYZ(it(pBackupVector, pBackupVector->end()))); } You can then e.g. iterator_range<it_PXYZ> getFiltered(std::vector<PXYZ> const& all) { std::vector<PXYZ> filtered; std::copy_if(all.begin(), all.end(), filtered.end(), ....); return iterators_from_copy<it_PXYZ>(filtered); }
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)) {...}
That is what I would do at the moment. By the way, I started a discussion on the topic at comp.lang.c++.isocpp.general - "Range-based for loop temporaries lifetime". There are some interesting comments on the subject there. Cheers, Filip
participants (5)
-
Filip Konvička
-
Gavin Lambert
-
Lee Clagett
-
Matei David
-
Neil Groves