[PATCH] D103900: [llvm] Add enum iteration to Sequence

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

Quuxplusone added a subscriber: lebedev.ri.
Quuxplusone added inline comments.


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp:87-91
+    // `index_begin` is unsigned(~0), so effectively iterating ~0, 0, 1, 2...
+    // This is why we need to use `SetIdx != EndIdx` instead of `SetIdx <
+    // EndIdx`.
+    for (unsigned SetIdx = AL.index_begin(), EndIdx = AL.index_end();
+         SetIdx != EndIdx; ++SetIdx) {
----------------
LGTM, but per @lebedev.ri 's comment in D83351, I think this would benefit from a follow-up patch that turns `index_begin` and `index_end` into `signed int`s.
Style nit: Personally I'd rather see "`unsigned(-1)`" and "`-1, 0, 1...`", than "`unsigned(~0)`" and "`~0, 0, 1...`".


================
Comment at: llvm/unittests/ADT/SequenceTest.cpp:118-119
+
+// DEATH tests
+#if !defined(NDEBUG)
+template <class T> class SequenceDeathTest : public SequenceTest<T> {
----------------
Pre-existing consistency issue: I see all of the following in llvm/unittests/, and they can't all be right:
```
#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST  // 1
#if defined(GTEST_HAS_DEATH_TEST) && !defined(NDEBUG)  // 2
#ifndef NDEBUG  // 3

and as a two-liner:

#ifdef GTEST_HAS_DEATH_TEST
#ifndef NDEBUG  // 4
```
In a vacuum, I'd recommend (2) above, because once the `#if` condition mentions `DEATH`, then your comment on line 118 becomes redundant and can be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103900