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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added a comment.

In D107378#3072867 <https://reviews.llvm.org/D107378#3072867>, @gchatelet wrote:

> In D107378#3072709 <https://reviews.llvm.org/D107378#3072709>, @aaron.ballman wrote:
>
>> Personally, I sort of wonder if it makes sense to to disallow enum iteration entirely. Are we convinced we want to encourage people to do this even when the enumeration is held just right?
>
> I don't think we want to encourage this pattern but when we have to use it I think it's better to have a tool to help with everything that can go wrong (integer promotion, non UB wrap around for signed types, reverse iteration, etc...).
> Being in a position where people re-implement it with subtle bugs is not great either.

Agreed!

> Now if enum iteration usage can be brought to 0 then yes we can certainly disallow it entirely.
> If not, maybe we want a separate interface to stress that it's an uncommon pattern (most of the underlying logic is shared with integer iteration so it would be best not to implement it twice).
>
> There are not a lot of places in llvm-project where we make use of enum iteration. MachineValueType <https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/MachineValueType.h#L1407-L1453> is probably the most pervasive. It relies on enum iteration to produce MVT or EVT on the fly.
> A quick search through the code base gives the following calls for `all_valuetypes()`, `integer_valuetypes()`, `fp_valuetypes()` and  `vector_valuetypes`.
>
>    % git grep all_valuetypes\(\) | grep -v include/llvm/Support/MachineValueType.h | wc -l
>   4
>    % git grep integer_valuetypes\(\) | grep -v include/llvm/Support/MachineValueType.h | wc -l
>   33
>    % git grep fp_valuetypes\(\) | grep -v include/llvm/Support/MachineValueType.h | wc -l
>   16
>    % git grep vector_valuetypes\(\) | grep -v include/llvm/Support/MachineValueType.h | wc -l
>   49
>
> If we can afford it, these functions can be implemented with static arrays (either lazily or eagerly initialized). Having real storage will allow reverse iteration which was the main motivation behind me touching this file D102679 <https://reviews.llvm.org/D102679>.
> Once these are gone, it may be possible to kill enum iteration entirely if we wish too.

Thank you for these details! You've convinced me it's worth having this interface for safety for the existing uses we aren't ready to refactor yet. I hadn't realized this was being used as much as it is in LLVM (I'm far more familiar with the Clang side of things, which I don't think does this often (ever?).) I think the approach you mention here:

> If you see this patch as overcomplicated, maybe it would make sense to keep the underlying iteration mechanism of iota_range in place, but disallow enum iteration with seq and implement it separately, with extra safety checks outside of iota_range?

makes sense to me. We make it a bit more clear that we expect enum iteration to be different from `seq` even if there's a shared underlying implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107378