14

Background and Previous Search

I'm looking for an elegant way to reverse-iterate over a container (e.g. std::vector) using a range-based for-loop in C++14. Searching for a solution I found this Q/A. It basically tells me, that this is not part of the standard library and I have to use boost or implement an adapter myself. I don't want to use boost, so I'm looking for the best own implementation now.

Besides the proposals given in previously mentioned Q/A, I also found this implementation and this blog regarding this topic. Most of the implementations are quite similar and seem quite decent. However they all have a pitfall: As pointed out in this comment you might end up with a dangling reference if you call the reverse-adapter with a temporary object:

for (const auto& v : reverse_iterate(getContainer()))

Regarding the problem with a temporary object in range-based for-loop, this answer really helped my understanding. But what can we do to prevent a dangling reference?

My Solution

Based on this background I'm searching for an implementation that get's rid of this pitfall. In the following implementation I'm using an additional rvalue-reference rx_ to prolong the lifetime of my input parameter iff reverse_iterate is called with rvalue reference.

EDIT: Do not use this solution. It is wrong as pointed out in accepted solution.

template <typename T>
class reverse_range
{
  T &&rx_; // rvalue-reference to prolong livetime of temporary object
  T &x_; // reference to container

public:
  explicit reverse_range(T &x) : rx_(T{}), x_(x) {}
  explicit reverse_range(T &&rx) : rx_(std::move(rx)), x_(rx_) {}

  auto begin() const -> decltype(this->x_.rbegin())
  {
    return x_.rbegin();
  }  
  auto end() const -> decltype(this->x_.rend())
  {
    return x_.rend();
  }
};

template <typename T>
reverse_range<T> reverse_iterate(T &x)
{
  return reverse_range<T>(x);
}
template <typename T>
reverse_range<T> reverse_iterate(T &&rx)
{
  return reverse_range<T>(std::move(rx));
}

Obviously we generate a little overhead of constructing an unused empty container object in the lvalue constructor, but I think that's not too bad. Besides one could probably get rid of this by providing two classes reverse_range_lvalue and reverse_range_rvalue, which each provide the implementation for one of the parameter types...

Questions

Would the above extension solve the dangling reference problem or do I miss something?

Do you have any hints on further problems regarding my code?

Are there better ideas to solve this problem in C++14 or any other (future) version?

Don-Umbro
  • 165
  • 9
  • 2
    *"rvalue-reference rx_ to prolong the lifetime"* will only work for local variables. (and should be avoided in general) – user7860670 Oct 09 '18 at 13:38
  • @VTT no, it will work for any reference. And will prologue the lifetime of the bound prvalue to the lifetime of the reference. Granted here the prvalue lifetime will be prolongued to the lifetime of the ctro reference parameter. – bolov Oct 09 '18 at 13:42
  • 1
    @bolov I'm pretty sure reference liftime extension explicitly states it doesn't work in certain situations. – Yakk - Adam Nevraumont Oct 09 '18 at 13:46
  • @Yakk-AdamNevraumont it's very possible I am wrong – bolov Oct 09 '18 at 13:50

1 Answers1

16

That doesn't work. Lifetime extension doesn't work in (that part of) constructors. (It works in the body of the constructor, just not in the member initializer list).

template<class R>
struct backwards_t {
  R r;
  constexpr auto begin() const { using std::rbegin; return rbegin(r); }
  constexpr auto begin() { using std::rbegin; return rbegin(r); }
  constexpr auto end() const { using std::rend; return rend(r); }
  constexpr auto end() { using std::rend; return rend(r); }
};
// Do NOT, I repeat do NOT change this to `backwards_t<std::decay_t<R>>.
// This code is using forwarding references in a clever way.
template<class R>
constexpr backwards_t<R> backwards( R&& r ) { return {std::forward<R>(r)}; }

this does a move when passed an rvalue, and keeps a reference when passed an lvalue.

The trick is that for a forwarding reference T&&, if T&& is an lvalue then T is a reference, and if T&& is an rvalue then T is a value. So we convert lvalues to references (and don't make a copy) while converting rvalues to values (and move the rvalue into said value).

for (const auto& v : backwards(getContainer()))

so that works.

In you can do a bit "better"; reference lifetime extension can apply to the content of structs if you do aggregate initialization. But I'd advise against it; reference lifetime extension is fragile and dangerous when it breaks.

There is talk in or later to permit compilers to convert moves into expiring objects into elisions. But I wouldn't bet on it working in an specific case. I also think I saw a paper about marking up ctors and functions with their lifetime dependency information (ie, that the return value depends on the lifetime of an argument), permitting warnings/errors and maybe more generalized lifetime extension.

So this is a known problem. But this is the best generally safe-ish way to solve this today.

Yakk - Adam Nevraumont
  • 262,606
  • 27
  • 330
  • 524
  • 3
    This is a very clever and elegant solution. +1 – NathanOliver Oct 09 '18 at 14:00
  • Thank you for this solution! Some additional questions for better understanding: ## You are using template reference forwarding like described here https://en.cppreference.com/w/cpp/language/reference#Forwarding_references ? ## The type `R` is something like `std::vector` for call with rvalue and something like `std::vector&` for call with lvalue? ## What benefit has `using std::rbegin` and similar? – Don-Umbro Oct 09 '18 at 14:58
  • @Don-Umbro the using permits ADL extension of `rbegin` for a custom type. I'm being clever with forwarding references and using them in a slightly unusual way; yes, for an rvalue vector `T` in` `T&&` is deduced to be `vector` and for an lvalue `T` is deduced to be `vector&`. – Yakk - Adam Nevraumont Oct 09 '18 at 15:03
  • @Yakk-AdamNevraumont are the curly braces in `return {std::forward(r)};` required? edit: ah i guess yes cause aggregate initialization? – phön Oct 09 '18 at 15:10
  • @phön I was lazy and didn't write a ctor for `backwards_t` based off the principle of "don't write code you don't need". Arguably I should remove the non-const `rbegin` and `rend` as well, as non-const backward iteration over an rvalue is probably a bug. I'll go fix that. Oh wait, no, what about moving out of the temporary container? Oh well, I'll just leave the two overloads there. – Yakk - Adam Nevraumont Oct 09 '18 at 15:18
  • If I use your code in Visual Studio 2015 I get following compiler errors and warnings referring to second `begin()`: ## `auto backwards_t::begin(void) const": Memberfunction already defined or declared` ## `backwards_t>::begin": In C++14 "constexpr" is not implicit "const". Use "const" explict.` – Don-Umbro Oct 09 '18 at 15:27
  • Same errors for end obviously. Any hints? I don't get it, because `const` should be valid override specification. – Don-Umbro Oct 09 '18 at 15:30
  • @Don-Umbro Your old compiler is making `constexpr` marked methods implicitly `const`. The easiest solution is to remove the word `constexpr` for now. Or tell compiler to use more recent C++ standard? – Yakk - Adam Nevraumont Oct 09 '18 at 15:31
  • Ahh, okay. Now it works. Is there any drawback I have to keep in mind when removing `constexpr`? I've only a vague idea about the benefit and usage of `constexpr`. – Don-Umbro Oct 09 '18 at 15:35
  • @Don-Umbro Well, if you had a constexpr array you wanted to iterate over backwards you couldn't use `backwards( some_array )` and have the resulting iteration be backwards. That is about it. – Yakk - Adam Nevraumont Oct 09 '18 at 15:42
  • 1
    could you please point to the description of the better reference lifetime extension in c++17? – Dev Null Oct 20 '18 at 20:51
  • 1
    @devnull the extension isn't new; rather, template constructor deduction together with pre-existing aggregate referencr member lifetime extension rules. It is quirky and fragile enough that I won't recommend it. But look into lifetime extension with aggregates in C++11. – Yakk - Adam Nevraumont Oct 20 '18 at 21:16