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

[Clement Courbet via Phabricator via llvm-commits]

courbet added inline comments.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:26
+struct value_sequence_iterator
+    : public std::iterator<std::forward_iterator_tag, const T> {
+  value_sequence_iterator(const T Value) : Value(Value) {}
----------------
why is this no longer a random access iterator ?


================
Comment at: llvm/include/llvm/ADT/Sequence.h:27
+    : public std::iterator<std::forward_iterator_tag, const T> {
+  value_sequence_iterator(const T Value) : Value(Value) {}
 
----------------
Let's keep the forwarnding reference ctor for when `T` is expensive:

```
template <typename U, typename Enabler = decltype(ValueT(std::declval<U>()))>
  value_sequence_iterator(U &&Value) : Value(std::forward<U>(Value)) {}
```



================
Comment at: llvm/include/llvm/ADT/Sequence.h:49
+
+template <typename T> struct value_sequence {
+  static_assert(std::is_same<T, std::remove_cv_t<T>>::value,
----------------
Does this need to be public ?


================
Comment at: llvm/include/llvm/ADT/Sequence.h:49
+
+template <typename T> struct value_sequence {
+  static_assert(std::is_same<T, std::remove_cv_t<T>>::value,
----------------
courbet wrote:
> Does this need to be public ?
should this be `value_range` ?


================
Comment at: llvm/include/llvm/ADT/Sequence.h:67
+
+  value_sequence(T Begin, T End) : Begin(Begin), End(End) {}
+
----------------
```
template <typename U, typename Enabler = decltype(ValueT(std::declval<U>()))>
value_sequence(U &&Begin, U&& end);
```

or even:

```
template <typename U1,, typename U2, typename Enabler = decltype(ValueT(std::declval<U1>())),typename Enabler = decltype(ValueT(std::declval<U2>()))>
value_sequence(U1 &&Begin, U2&& end);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102679