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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet marked an inline comment as done.
gchatelet added inline comments.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:37
 
-private:
-  struct Forward {
-    static void increment(T &V) { ++V; }
-    static void decrement(T &V) { --V; }
-    static void offset(T &V, difference_type Offset) { V += Offset; }
-    static T add(const T &V, difference_type Offset) { return V + Offset; }
-    static difference_type difference(const T &A, const T &B) { return A - B; }
-  };
-
-  struct Reverse {
-    static void increment(T &V) { --V; }
-    static void decrement(T &V) { ++V; }
-    static void offset(T &V, difference_type Offset) { V -= Offset; }
-    static T add(const T &V, difference_type Offset) { return V - Offset; }
-    static difference_type difference(const T &A, const T &B) { return B - A; }
-  };
-
-  using Op = std::conditional_t<!IsReversed, Forward, Reverse>;
-
-public:
+struct StrongInt {
   // default-constructible
----------------
courbet wrote:
> `OverflowCheckingInt` ?
> `OverflowCheckingInt` ?

I went with `SafeInt` as it's shorter and conveys the idea.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:82
+
+  // Integral constructor, asserts if Value cannot be represented as intmax_t.
+  template <typename Integral, typename std::enable_if_t<
----------------
courbet wrote:
> so `uintmax_t` is no longer supported ? This should be noted somewhere. I'm wondering whether this is an issue or not...
> so `uintmax_t` is no longer supported ? This should be noted somewhere. I'm wondering whether this is an issue or not...

It is supported but not all of its values (no values above `INTMAX_MAX` are permitted).

I believe this is not a problem as this class is really about iterating values. It would be surprising to iterate values between `INTMAX_MAX + 1` and `UINTMAX_MAX`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106279