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

[Arthur O'Dwyer via Phabricator via llvm-commits]

Quuxplusone added inline comments.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:26-28
+    : public std::iterator<std::random_access_iterator_tag, T, ptrdiff_t> {
+private:
+  using Base = std::iterator<std::random_access_iterator_tag, T, ptrdiff_t>;
----------------
Lines 26 and 28, please avoid `std::iterator` because it is deprecated in C++17 and I assume we anticipate compiling LLVM with `-std=c++17` in the near future. Just write out the 5 public member typedefs by hand. (Actually you already write out `difference_type`, so there's only 4 typedefs left. ;))


================
Comment at: llvm/include/llvm/ADT/Sequence.h:48
+  };
+  using Op = std::conditional_t<IsForward, Forward, Backward>;
 
----------------
Naming nit: When I saw `Forward` in an iterator context, I assumed you were talking about forward iterators versus bidirectional iterators, and was going to say "technically, bidirectional iterators are //also// forward iterators." But on second glance I see that by `IsForward` you really mean `IsNotReversed`. I suggest `s/IsForward/!IsReversed/g` and `s/Backward/Reverse/g`.

Orthogonally, I didn't realize until `operator[]` that by `value_range` you meant not just "`operator*` returns by value," but actually `iota_range`; and I wonder if that would be a good renaming.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:56-58
+  template <typename U,
+            std::enable_if_t<std::is_convertible<U, T>::value, bool> = true>
+  value_range_iterator(U &&Value) : Value(std::forward<U>(Value)) {}
----------------
I suspect this should be just
```
explicit value_range_iterator(T Value) : Value(Value) {}
```
All constructors (except copy and move) should always be `explicit`; and I don't think the templatey stuff is serving a purpose here.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:67
+
+  value_sequence(T Begin, T End) : Begin(Begin), End(End) {}
+
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102679