[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:35
+  struct Forward {
+    static void Inc(T &V) { ++V; }
+    static void Dec(T &V) { --V; }
----------------
style (here and below)


================
Comment at: llvm/include/llvm/ADT/Sequence.h:146
+template <typename ValueT> struct value_range {
+  using value_type = const ValueT;
+  using reference = ValueT &;
----------------
why const ? This makes `value_range` not movable.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:178
+template <typename ValueT> auto seq(ValueT Begin, ValueT End) {
+  return value_range<ValueT>(Begin, End);
 }
----------------
`std::move(Begin), std::move(End)` if you go with the forwarding reference ctors


================
Comment at: llvm/include/llvm/ADT/Sequence.h:67
+
+  value_sequence(T Begin, T End) : Begin(Begin), End(End) {}
+
----------------
gchatelet wrote:
> 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;
>   ^
> ```
Given that `ValueT` is a template parameter it is generally a good practice to not assume anything about the cost of copying. 
We might have a cheaply movable but expensive to copy `ValueT` for example. A user might want to move the bounds into the value range.

(And BTW given that the iterator holds a member variable of type `ValueT`, the iterator is not necessarily cheap to copy, so we might as well try to avoid the copies).

that being said I only see `seq` currently being used with integers, so your call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102679