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

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

Quuxplusone added inline comments.


================
Comment at: llvm/include/llvm/ADT/Sequence.h:143-149
+template <typename T, bool IsEnum = std::is_enum<T>::value>
+struct StorageType {};
+
+template <typename T> struct StorageType<T, false> { using type = T; };
+template <typename T> struct StorageType<T, true> {
+  using type = typename std::underlying_type<T>::type;
 };
----------------
gchatelet wrote:
> Quuxplusone wrote:
> > `llvm::detail::StorageType` is too generic of a name to land-grab, IMHO. This can/should be an implementation detail of `iota_range_iterator` — maybe it doesn't even need to be a template parameter.
> > ```
> > template<class T>
> > struct iota_range_iterator {
> >     using storage_type = typename std::conditional_t<std::is_enum<T>::value,
> >         type_identity<T>, std::underlying_type<T>>::type;
> > ```
> > Technically, instantiating underlying_type<T> for non-enum T was UB before C++20.
> > In practice I'd hope it works on all implementations we care about.
> > I don't know where to get a C++14-friendly `type_identity` but hopefully you can figure it out.
> > https://en.cppreference.com/w/cpp/types/type_identity
> > `llvm::detail::StorageType` is too generic of a name to land-grab, IMHO. This can/should be an implementation detail of `iota_range_iterator` — maybe it doesn't even need to be a template parameter.
> > Technically, instantiating underlying_type<T> for non-enum T was UB before C++20.
> > In practice I'd hope it works on all implementations we care about.
> 
> Unfortunately no, `std::conditional` must evaluated both types, and `std::underlying_type<T>` does not provide the type member when `T` is not an enum. The standard seems to specify this use case explicitly.
> https://en.cppreference.com/w/cpp/types/underlying_type
> 
> I had to provide type resolution via template specialization.
> Because of how nested template specialization works, I can't make `StorageType` part of `iota_range_iterator` either so it has to live outside of `iota_range_iterator`.
> 
> I tried many different solutions but I couldn't make it much better.
> 
> I renamed `StorageType` to `iota_range_iterator_storage_type` so at least it's less susceptible to accidental hijacking.
> 
> > I don't know where to get a C++14-friendly `type_identity` but hopefully you can figure it out.
> > https://en.cppreference.com/w/cpp/types/type_identity
> 
> `type_identity` doesn't help in that case.
> 
> 
I'd mixed up the arguments to `conditional_t`. Try this:
https://godbolt.org/z/q9fxznja7


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103900