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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet marked 3 inline comments as done.
gchatelet 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) {}
----------------
courbet wrote:
> why is this no longer a random access iterator ?
`forward_iterator` ought to be enough for everyone : )
More seriously I first thought it would suffice and it does for all of llvm, except MLIR. It does something funky with its metadata and [[ https://github.com/llvm/llvm-project/blob/731206f3684af5979e3a794970db83f9a34b4541/mlir/include/mlir/IR/BuiltinAttributes.h#L801 | assumes that the underlying iterator is random access ]]. I've implemented the random access iterator trait but it makes the code more complex.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:67
+
+  value_sequence(T Begin, T End) : Begin(Begin), End(End) {}
+
----------------
courbet wrote:
> ```
> 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);
> ```
This introduces some resolution issues and I'm not sure this brings much.
Iterators must be lightweight and copyable by definition.

**edit:** This one can actually be solved by the second version with two typename template parameters. It still feels a bit overkill to me but let me know it you think it's worth it.

```
In file included from /redacted/git/llvm-project/llvm/lib/Analysis/LoopCacheAnalysis.cpp:30:
/redacted/git/llvm-project/llvm/include/llvm/ADT/Sequence.h:86:41: error: no matching constructor for initialization of 'llvm::value_range<unsigned int>::const_iterator' (aka 'value_range_iterator<unsigned int, true>')
  const_iterator begin() const { return {Begin}; }
                                        ^~~~~~~
/redacted/git/llvm-project/llvm/lib/Analysis/LoopCacheAnalysis.cpp:169:20: note: in instantiation of member function 'llvm::value_range<unsigned int>::begin' requested here
  for (auto SubNum : seq<unsigned>(0, NumSubscripts - 1)) {
                   ^
/redacted/git/llvm-project/llvm/include/llvm/ADT/Sequence.h:30:3: note: candidate constructor not viable: no known conversion from 'llvm::value_range<unsigned int>::value_type' (aka 'const unsigned int') to 'const llvm::detail::value_range_iterator<unsigned int, true>' for 1st argument
  value_range_iterator(const value_range_iterator &) = default;
  ^
/redacted/git/llvm-project/llvm/include/llvm/ADT/Sequence.h:33:3: note: candidate template ignored: substitution failure [with U = const unsigned int &]: use of undeclared identifier 'ValueT'
  value_range_iterator(U &&Value) : Value(std::forward<U>(Value)) {}
  ^
/redacted/git/llvm-project/llvm/include/llvm/ADT/Sequence.h:28:3: note: candidate constructor not viable: requires 0 arguments, but 1 was provided
  value_range_iterator() = default;
  ^
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102679