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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Do you/we have a use for this machinery in mind? (especially the extra `forge` pieces - probably nice to leave that out until/unless it's needed - maybe even if it is needed, might be easier to review separately/in a follow-up patch)



================
Comment at: llvm/include/llvm/ADT/Sequence.h:54-60
+// 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, ...) {
+  static_assert(std::is_enum<EnumT>::value, "Not an enum type");
+  static_assert(detail::always_false<EnumT>::value,
+                "add_enum_triats not defined for this enum type");
+}
----------------
If the goal is for this to be a compilation error if it's called, then maybe it should be defined as deleted instead of the static_asserts?
```
template<typename EnumT>
void add_enum_traits(EnumT value) = delete;
```



================
Comment at: llvm/include/llvm/ADT/Sequence.h:67-83
+// Use to declare ENUM as safe to iterate over. This can be used for enums
+// declared in namespace scope and inside class/struct.
+// We mark it as potentially unused to silence false positives.
+#define DECLARE_ITERABLE_ENUM(ENUM)                                            \
+  inline auto LLVM_ATTRIBUTE_UNUSED add_enum_traits(ENUM value) {              \
+    return llvm::forge_iterable_enum(value);                                   \
+  }
----------------
Why are both of these necessary? Presumably there's a default (which is either iterable or not iterable) and there's only a need to declare the non-default case?

Also it'd be preferable if the syntax for this could be simplified to the point where it didn't need a macro.

I'd have thought something like using classic C++ traits:
```
struct EnumTraits<MyEnum> : std::true_type { };
```

Might be adequate.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:153
   Enum to() const {
-    using type = typename std::underlying_type<Enum>::type;
+    using type = std::underlying_type_t<Enum>;
     return Enum(to<type>());
----------------
Is this (& similar changes) unrelated to this patch? It should be committed separately if that's the case/if it can be done separately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107378