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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added a comment.

@Quuxplusone how about renaming `seq` into `iota` then?
In a second NFC patch possibly to keep it simple.

We may also provide an overloaded version that implicitly starts at 0 (e.g. `iota(5) -> 0, 1, 2, 3, 4`).
More than half of the call sites use `0` explicitly as a first parameter.



================
Comment at: llvm/include/llvm/ADT/Sequence.h:67
+
+  value_sequence(T Begin, T End) : Begin(Begin), End(End) {}
+
----------------
Quuxplusone wrote:
> courbet wrote:
> > 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.
> ```
> explicit value_range(value_type Begin, value_type End)
>     : Begin(Begin), End(End) {}
> ```
> Based on @courbet's comment, I also suggest adding `static_assert(std::is_integral<ValueT>::value, "");` into this code, as a speed bump for anyone who tries to use it with non-integral types (because that's not currently a supported nor tested use-case, and they //ought// to think about it before plowing ahead blindly).
SGTM I have a use case that is not a primitive scalar value but a thin wrapper around it. I'll update the static_assert accordingly then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102679