[PATCH] D107350: Add a trait to Sequence to iterate enums

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added a comment.

In D107350#2923018 <https://reviews.llvm.org/D107350#2923018>, @kuhar wrote:

> What if instead we put traits together with enum values, and have a function (customization point) to augment an enum value with its traits? Then we can have a way to opt into enum iteration even for enums that aren't declared as iterable. I made a quick proof of concept and uploaded it here: https://reviews.llvm.org/D107378.

Thx for the patch.
I gave it a try and it seems it does not work in the context of the tests because of "warning as errors".

  /redacted/git/llvm-project/llvm/unittests/ADT/SequenceTest.cpp:210:1: error: unused function 'add_enum_traits' [-Werror,-Wunused-function]
  DECLARE_ITERABLE_ENUM(UntypedEnum)
  ^
  /redacted/git/llvm-project/llvm/include/llvm/ADT/Sequence.h:52:15: note: expanded from macro 'DECLARE_ITERABLE_ENUM'
    inline auto add_enum_traits(ENUM value) {                                    \
                ^

Unfortunately marking the function as `static` will not work when the function is declared in a class (`static` has a different meaning there)

  /redacted/git/llvm-project/llvm/include/llvm/Support/MachineValueType.h:1406:12: error: 'static' is invalid in friend declarations
      friend DECLARE_ITERABLE_ENUM(MVT::SimpleValueType)
             ^
  /redacted/git/llvm-project/llvm/include/llvm/ADT/Sequence.h:52:3: note: expanded from macro 'DECLARE_ITERABLE_ENUM'
    static inline auto add_enum_traits(ENUM value) {                                    \
    ^

The idea is interesting though, maybe there's a way to make it work? We could use different macros depending on context but ideally a single one would be best.

AFAIU the proposal is about making sure that we don't iterate on an enum by mistake but in your patch it seems that calling `make_iterable_enum` bypasses this security (i.e. X86::CondCode has never been declared iterable <https://reviews.llvm.org/D107378#change-DgLEQ1SuLFJw> ). Was that intended?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107350