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

Arthur O'Dwyer via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 9 06:43:05 PDT 2021


Quuxplusone accepted this revision.
Quuxplusone added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:153
+  using const_reverse_iterator = reverse_iterator;
+  using difference_type = typename iterator::difference_type;
+  using size_type = std::size_t;
----------------
Nit: Just `std::ptrdiff_t` seems simpler for the reader.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:159-164
+  template <
+      typename BeginT, typename EndT,
+      std::enable_if_t<std::is_convertible<BeginT, ValueT>::value, bool> = true,
+      std::enable_if_t<std::is_convertible<EndT, ValueT>::value, bool> = true>
+  iota_range(BeginT &&Begin, EndT &&End)
+      : Begin(std::forward<BeginT>(Begin)), End(std::forward<EndT>(End)) {}
----------------
I still believe this should be simply
```
explicit iota_range(ValueT Begin, ValueT End)
      : Begin(Begin), End(End) {}
```
with no templates. (For the same reason as lines 182-183.)
And `explicit` because all constructors should be explicit.


================
Comment at: llvm/unittests/ADT/SequenceTest.cpp:42
 
-  auto adjusted_begin = my_seq.begin() + 2;
-  auto adjusted_end = my_seq.end() - 2;
-  EXPECT_TRUE(adjusted_begin == adjusted_end);
-  EXPECT_EQ(2, *adjusted_begin);
-  EXPECT_EQ(2, *adjusted_end);
+TEST(SequenceTest, Dereferene) {
+  const auto Forward = seq(0, 10).begin();
----------------
/Dereferene/Dereference/


================
Comment at: mlir/include/mlir/IR/BuiltinAttributes.td:710
     using iterator =
-        llvm::mapped_iterator<llvm::detail::value_sequence_iterator<ptrdiff_t>,
+        llvm::mapped_iterator<typename decltype(llvm::seq<ptrdiff_t>(0, 0))::const_iterator,
                               std::function<T(ptrdiff_t)>>;
----------------
gchatelet wrote:
> The purpose of this change was to prevent mlir from depending on implementation details of llvm.
> 
> @mehdi_amini I've added `typename` to disambiguate between value and type for GCC5 but I have no way of checking whether that works or not (GCC5 is not available on my machine).
> Do you have any suggestions on how to test it?
> 
Indeed the error message looked to me like the symptom of "forgetting to include the right header somewhere," because the compiler seemed to be complaining that it didn't know what `ptrdiff_t` meant. Adding `typename` here also LGTM.
Nit: `const_iterator` could be just `iterator`, right? There's no difference between const-iterating and regular-iterating an `llvm::seq`, so the difference between them is nonexistent and we might as well use the shorter simpler name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103948



More information about the llvm-commits mailing list