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

[Clement Courbet via Phabricator via llvm-commits]

courbet accepted this revision.
courbet added inline comments.
This revision is now accepted and ready to land.


================
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); }
----------------
gchatelet wrote:
> 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.
Makes sense, thanks for the explanation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102679