idaholab / moose

Multiphysics Object Oriented Simulation Environment
https://www.mooseframework.org
GNU Lesser General Public License v2.1
1.66k stars 1.02k forks source link

Add static reflection for enums to construct matching MooseEnums #17810

Open dschwen opened 3 years ago

dschwen commented 3 years ago

Reason

We often use a pattern where a MooseEnum parameter is fetched and immediately converted into an enum class object. If the labels for the two enum types do not match very confusing bugs can be introduced.

Design

We could use static reflection to construct a MooseEnum from an existing enum class, setting all the valid items to the string representations of the enum class items. The construction of such a MooseEnum could look like this

enum class Options {
   FIRST_OPTION, SECOND_OPTION, NONE
};
auto optionEnum = MooseEnum::fromEnum<Options>();

which would give the same result as

MooseEnum optionEnum("FIRST_OPTION SECOND_OPTION NONE");

(I'm sure there is support for specific underlying type values (FIRST_OPTION=1, SECOND_OPTION=2, NONE=0) and, an option to specify the default value auto optionEnum = MooseEnum::fromEnum<Options>(Option::NONE); could be added as well).

The library to do this is https://github.com/Neargye/magic_enum, however, it would require us to raise the minimum compiler requirements, and it would rely on non-standard, albeit well supported, compiler features.

I'm opening this issue, because the question of enum consistency has come up multiple times now (I believe by @tophmatthews and @hugary1995), and I want some permanent record for when we arrive at sufficiently recent minimum supported compiler versions.

Impact

New API to enforce consistency between corresponding enum type eliminates a source for bugs.

roystgnr commented 3 years ago

That library requires not only C++17, but also

a compiler-specific hack (based on __PRETTY_FUNCTION__ / __FUNCSIG__)

Are you sure you didn't get the title correct the first time?

:drum: :bell:

roystgnr commented 3 years ago

More seriously, though - we could use this immediately for verifying enum strings, even if we can't rely on it for generating them.

If we can get constexpr outputs from this, then we can stick those in a comparison in a static_assert so that the hand-written strings get verified with every CI build on a hack-supporting compiler, and we can wrap all that in some #ifdef ENUM_REFLECTION_HACK as well as #if __cplusplus > 201703L so it doesn't break on random non-C++17 or non-hack-supporting compilers.

roystgnr commented 3 years ago

Less seriously again - is there really no better emoticon to use for the crash following a rimshot? Lacking a simple cymbal symbol is some bull.

dschwen commented 2 years ago

The enum_name function is indeed constexpr, but we'd also need a compile time way to split strings along spaces and iterator over them. This can probably be done with parameter packs and tons of template magic. But I'll hold off on this until we support C++17.

Maybe we should add a label for issues that are waiting on C++17...

loganharbour commented 2 years ago

Maybe we should add a label for issues that are waiting on C++17...

I like this. Will help us keep track when we finally get it going

loganharbour commented 2 years ago

I'd like to note that we do actually have c++17 with the default compiler on the build boxes, so there isn't anything stopping you from testing the majority of the stuff on next right now

roystgnr commented 2 years ago

Yeah, not only that but our min clang tests are getting run on C++17, so you should be able to differentiate the different types of failure: Min gcc failure is expected and means you're actually using C++17, Min clang failure means you accidentally used some C++20 or something, other failures mean actual failures.