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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a subscriber: aaron.ballman.
dblaikie added a comment.

@jkuhar - is there some data you could include in the patch description/commit message to help explain/motivate this functionality? Common bugs (links to commits that fixed bugs that would've been found earlier if we had this functionality would be good) introduced by not having this safety/protection over iterating over enums?

@dexonsmith @aaron.ballman - curious what you two think of this for general design. Reckon it's worth the macros and novel traits system (using an inline function rather than/in addition to a template specialization) to allow the trait to be specified closer to the type in nested type situations?



================
Comment at: llvm/include/llvm/ADT/Sequence.h:9-33
+/// Provides some synthesis utilities to produce sequences of values. The names
+/// are intentionally kept very short as they tend to occur in common and
+/// widely used contexts.
+///
+/// The `seq(A, B)` function produces a sequence of values from `A` to up to
+/// (but not including) `B`, i.e., [`A`, `B`), that can be safely iterated over.
+/// `seq` supports both integral (e.g., `int`, `char`, `uint32_t`) and enum
----------------
might be good to commit this part separately before the rest, since it's not related to the enum stuff being added in the rest of this patch?


================
Comment at: llvm/include/llvm/ADT/Sequence.h:102
+  using enum_type = Enum;
+  using underlying_type = std::underlying_type_t<Enum>;
+  using traits_type = Traits;
----------------
Looks like this is unused and could be removed/leave it to users of `enum_with_traits` to compose this themselves out of `enum_type` and `std::underlying_type_t`.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:103
+  using underlying_type = std::underlying_type_t<Enum>;
+  using traits_type = Traits;
+};
----------------
Not sure that bundling all these things together's going to be helpful or more complicated. I think there's probably value in this code looking as much as possible like classic C++ traits (with the exception of using an inline function to retrieve the traits if needed/if that's the direction this goes)

So maybe changing "enum_with_traits add_enum_traits(Enum)" to "traits get_enum_traits(Enum)".

This might allow the "escape hatch" part of this patch to go in separately from the first part of the patch (unless existing users need the escape hatch? - though at least even then the escape hatch functionality could be separate/easier to understand/test/implement in isolation from the rest). Since the `iota_range<Enum, std::enable_if_t<std::is_enum<Enum>::value>>` is separate from the `iota_range<enum_with_traits<Enum, Traits>, ...>` specialization anyway - I'm not sure the implementation benefits from implementation details that overlap between these two things?


================
Comment at: llvm/include/llvm/ADT/Sequence.h:122
+  inline auto LLVM_ATTRIBUTE_UNUSED add_enum_traits(ENUM value) {              \
+    return llvm::forge_iterable_enum(value);                                   \
+  }
----------------
I think for usability (error message quality, etc) it might be worth not using `forge_iterable_enum` here, since if it appears in an error it might be confusing to users since this type isn't being forged, it's using the legitimate entry point for a type truly declared iterable. (so maybe the implementation should be `return enum_with_traits<Enum, iterable_enum_traits<Enum>>(Value);`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107378