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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet 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;
 };
----------------
Quuxplusone wrote:
> 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
oh... my apologies. I should have checked as well. I had convinced myself that the error was `std::conditional_t` needing to resolve both types ...

So I had to provide a C++14 `type_identity` but it works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103900