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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet marked 3 inline comments as done.
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:
> `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.




================
Comment at: llvm/include/llvm/ADT/Sequence.h:203
+  auto result = seq(Begin, End);
+  result.End = *++result.end();
+  return result;
----------------
Quuxplusone wrote:
> Phab somehow ate my comment here.
> This should be just `return seq(Begin, static_cast<T>(End + 1));`
> and then in the constructor of `iota_range`, please `assert(Begin <= End)` just as a sanity check.
> Phab somehow ate my comment here.
> This should be just `return seq(Begin, static_cast<T>(End + 1));`

Because `End` may be a scoped enum `End + 1` is not always a valid statement.
Trying to do the arithmetic here will yield the same problems that `IterableEnum` was supposed to solve.
The best we could do here would be something along those lines:

```
return seq(Begin, static_cast<T>(static_cast<detail::StorageType<T>::type>(End) + 1));
```

Which, IMHO, is redoing most of the work that has been moved to `iota_range_iterator` (hence the use of iterators to bump the values)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103900