[PATCH] D102679: [llvm] Make Sequence reverse-iterable

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added inline comments.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:93
+  // Dereference
+  T operator*() const { return Value; }
+  T operator[](difference_type Offset) const { return Op::add(Value, Offset); }
----------------
courbet wrote:
> This is formally OK given that the return value `T` verifies `__Referenceable<T>`, but is there any reason not to return a `const T&` ?
> (Again, this is mostly a theoretical argument, feel free to ignore.)
Yes there is a reason.
A typical range or container holds all of its values and iterators refer to them, so it makes sense to return a reference because the value outlives the iterator.
In this particular case the iterator contains the value which might lead to dangling references.

In fact this is why I had to provide the reverse iterators in the first place. With the original `Sequence` code, the std::reverse_iterator's pre-increment operator generated from `Sequence::iterator` behaves like a post-increment operator and copies the iterator. Pseudo code:
```
  reverse_iterator operator++(int) {
    reverse_iterator Tmp = *this;
    ++(*this);
    return Tmp;
  }
```

In the following code, `value` ends up being a dangling reference.
```
auto sequence = std::reverse(llvm::Seq(0, 10));
const auto& value = *++sequence.begin();
```

Using a value type instead of a reference type should trigger an error from the compiler in such cases.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102679/new/

https://reviews.llvm.org/D102679