[PATCH] D107378: Make enum iteration with seq safe by default

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added inline comments.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:25
 #include "llvm/Support/MathExtras.h" // AddOverflow / SubOverflow
 
 namespace llvm {
----------------
Since the API changed and it's now less easy to iterate over enums can you provide a documentation block explaining the different ways of iterating over numbers and enums?


================
Comment at: llvm/include/llvm/ADT/Sequence.h:54-55
+
+// Default implementation of (unimplemented) add_enum_traits. This should the
+// worst overload that, when selected, produces a compilation error.
+template <typename EnumT> auto add_enum_traits(EnumT value, ...) {
----------------
A word seems to be missing, also could you rephrase, it's not super clear.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:263-266
   static_assert(std::is_integral<T>::value || std::is_enum<T>::value,
                 "T must be an integral or enum type");
-  static_assert(std::is_same<T, std::remove_cv_t<T>>::value,
+  static_assert(std::is_same<value_type, std::remove_cv_t<value_type>>::value,
                 "T must not be const nor volatile");
----------------
Either use `T` everywhere or `value_type` everywhere (including in the error message).


================
Comment at: llvm/include/llvm/ADT/Sequence.h:291
+    auto WithTraits = add_enum_traits(Begin);
+    (void)WithTraits;
+    using traits = typename decltype(WithTraits)::traits_type;
----------------
Why is this necessary? Can you add a comment?


================
Comment at: llvm/include/llvm/ADT/Sequence.h:292
+    (void)WithTraits;
+    using traits = typename decltype(WithTraits)::traits_type;
+    static_assert(
----------------
Maybe just inline


================
Comment at: llvm/unittests/ADT/SequenceTest.cpp:235-236
+TEST(SequenceTest, IterableEnums) {
+  EXPECT_TRUE(llvm::empty(
+      llvm::iota_range<UntypedEnum>(UntypedEnum::A, UntypedEnum::A, false)));
+  EXPECT_THAT(
----------------
Here and below. in the empty case you can use the gmock matcher `IsEmpty` ([[ http://google.github.io/googletest/reference/matchers.html#container-matchers | documentation ]])
```EXPECT_THAT(llvm::iota_range(UntypedEnum::A, UntypedEnum::A, false), IsEmpty());```

It's not exactly the same semantic as it relies on the `empty()` member function instead of testing that `begin` and `end` are equal, but the test is more symmetric and easier to read.


================
Comment at: llvm/unittests/ADT/SequenceTest.cpp:236
+  EXPECT_TRUE(llvm::empty(
+      llvm::iota_range<UntypedEnum>(UntypedEnum::A, UntypedEnum::A, false)));
+  EXPECT_THAT(
----------------
You shouldn't need to qualify the template argument here and below. This would help readability.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107378