Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[RFC] Rich enum constants without parens #136

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[RFC] Rich enum constants without parens #136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 50 additions & 4 deletions include/fixed_containers/enum_utils.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <magic_enum.hpp>

#include <array>
#include <compare>
#include <concepts>
#include <cstddef>
#include <functional>
Expand Down Expand Up @@ -364,14 +365,59 @@ class SkeletalRichEnumStorageBase<RichEnumType,

} // namespace fixed_containers::rich_enums_detail

template <typename RichEnumType>
class RichEnumConstantProxy
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally approve of this idea. I am leaving my thoughts as a comment rather than as PR review such that replies are simpler.

Specifically, I like that this change makes the common use cases of RichEnums simpler. For example, I like that we can omit operator() when comparing enums such as:

TestRichEnum1::value_of(BE::C_ONE) == TestRichEnum1::C_ONE

or in switch statements such as:

case TestRichEnum1::C_ONE:

However, the current approach has some downsides in my eyes.

  1. The first downside in my opinion is that calling member functions of RichEnums now requires operator->, which in my opinion is an operator which conveys pointer like semantics which imply nullptr possibility. However, since the pointer will never be null in this case, this is a purely aesthetic concern and unfortunately I don't currently have alternate suggestions. Example:
TestRichEnum1::C_FOUR->ordinal()
  1. I am not in favor of the
// TRANSITION
constexpr const RichEnumType& operator()()

In my opinion, this will result in having two different ways of doing the same thing and I expect both to stick around. From what I've seen, things in transition:: tend to stick around ;) . I suggest that we make this a breaking change that requires users to change their code. That is unless this repository has some convention of backwards compatibility.

{
private:
using BackingEnum = typename RichEnumType::BackingEnum;

BackingEnum backing_enum_{};

public:
explicit(false) constexpr RichEnumConstantProxy(const BackingEnum& backing_enum)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question from curiosity: I've noticed the pattern explicit(false) for a few ctors. What is the reason for it? Omitting it would have no effect since ctors are implicit by default, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explicit(false) makes it explicit that the function is intended to allow implicit conversion and it is not an omission.

: backing_enum_{backing_enum}
{
}

constexpr RichEnumConstantProxy(const RichEnumConstantProxy& original) noexcept = delete;
constexpr RichEnumConstantProxy(RichEnumConstantProxy&& RichEnumConstantProxy) noexcept =
delete;
constexpr RichEnumConstantProxy& operator=(const RichEnumConstantProxy& RichEnumConstantProxy) =
delete;
constexpr RichEnumConstantProxy& operator=(RichEnumConstantProxy&& original) = delete;

constexpr const RichEnumType& get() const
{
return ::fixed_containers::rich_enums_detail::value_of<RichEnumType>(backing_enum_)
.value()
.get();
}

explicit(false) constexpr operator BackingEnum() const { return get(); }
explicit(false) constexpr operator const RichEnumType&() const { return get(); }

// TRANSITION
constexpr const RichEnumType& operator()() const { return get(); }

constexpr bool operator==(const RichEnumType& other) const { return get() == other; }
constexpr std::strong_ordering operator<=>(const RichEnumType& other) const
{
return get() <=> other;
}

constexpr const RichEnumType* operator->() const { return std::addressof(get()); }
constexpr const RichEnumType& operator*() const noexcept { return get(); }
constexpr const RichEnumType* operator&() const noexcept // NOLINT(google-runtime-operator)
{
return std::addressof(get());
}
};

// MACRO to reduce four lines into one and avoid bugs from potential discrepancy between the
// BackingEnum::ENUM_CONSTANT and the rich enum ENUM_CONSTANT()
// Must be used after the values() static function is declared in the rich enum.
#define FIXED_CONTAINERS_RICH_ENUM_CONSTANT_GEN_HELPER(RichEnumName, CONSTANT_NAME) \
static constexpr const RichEnumName& CONSTANT_NAME() \
{ \
return RichEnumName::value_of(BackingEnum::CONSTANT_NAME).value(); \
}
static constexpr RichEnumConstantProxy<RichEnumName> CONSTANT_NAME{BackingEnum::CONSTANT_NAME};

namespace fixed_containers::rich_enums
{
Expand Down
102 changes: 51 additions & 51 deletions test/enum_utils_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ static_assert(IsRichEnumStorage<RichEnumStorage<TestRichEnumBoolBackingEnum>>);
TEST(RichEnum, DirectFieldAccess)
{
static_assert(
consteval_compare::equal<rich_enums::TestRichEnum1::C_ONE().detail_backing_enum.val,
consteval_compare::equal<rich_enums::TestRichEnum1::C_ONE->detail_backing_enum.val,
TestRichEnum1BackingEnum::C_ONE>);
}

TEST(RichEnum, DirectFieldAccessBool)
{
static_assert(
consteval_compare::equal<rich_enums::TestRichEnumBool::TRUE_VALUE().detail_backing_enum.val,
consteval_compare::equal<rich_enums::TestRichEnumBool::TRUE_VALUE->detail_backing_enum.val,
TestRichEnumBoolBackingEnum::TRUE_VALUE>);
}

Expand Down Expand Up @@ -159,10 +159,10 @@ TEST(BuiltinEnumAdapter, Ordinal)
TEST(RichEnumAdapter, Ordinal)
{
static_assert(4 == EnumAdapter<TestRichEnum1>::count());
static_assert(0 == EnumAdapter<TestRichEnum1>::ordinal(TestRichEnum1::C_ONE()));
static_assert(1 == EnumAdapter<TestRichEnum1>::ordinal(TestRichEnum1::C_TWO()));
static_assert(2 == EnumAdapter<TestRichEnum1>::ordinal(TestRichEnum1::C_THREE()));
static_assert(3 == EnumAdapter<TestRichEnum1>::ordinal(TestRichEnum1::C_FOUR()));
static_assert(0 == EnumAdapter<TestRichEnum1>::ordinal(TestRichEnum1::C_ONE));
static_assert(1 == EnumAdapter<TestRichEnum1>::ordinal(TestRichEnum1::C_TWO));
static_assert(2 == EnumAdapter<TestRichEnum1>::ordinal(TestRichEnum1::C_THREE));
static_assert(3 == EnumAdapter<TestRichEnum1>::ordinal(TestRichEnum1::C_FOUR));
}

TEST(SpecializedEnumAdapter, Ordinal)
Expand Down Expand Up @@ -190,10 +190,10 @@ TEST(BuiltinEnumAdapter, ToString)
TEST(RichEnumAdapter, ToString)
{
static_assert(4 == EnumAdapter<TestRichEnum1>::count());
static_assert("C_ONE" == EnumAdapter<TestRichEnum1>::to_string(TestRichEnum1::C_ONE()));
static_assert("C_TWO" == EnumAdapter<TestRichEnum1>::to_string(TestRichEnum1::C_TWO()));
static_assert("C_THREE" == EnumAdapter<TestRichEnum1>::to_string(TestRichEnum1::C_THREE()));
static_assert("C_FOUR" == EnumAdapter<TestRichEnum1>::to_string(TestRichEnum1::C_FOUR()));
static_assert("C_ONE" == EnumAdapter<TestRichEnum1>::to_string(TestRichEnum1::C_ONE));
static_assert("C_TWO" == EnumAdapter<TestRichEnum1>::to_string(TestRichEnum1::C_TWO));
static_assert("C_THREE" == EnumAdapter<TestRichEnum1>::to_string(TestRichEnum1::C_THREE));
static_assert("C_FOUR" == EnumAdapter<TestRichEnum1>::to_string(TestRichEnum1::C_FOUR));
}

TEST(SpecializedEnumAdapter, ToString)
Expand All @@ -208,73 +208,73 @@ TEST(SpecializedEnumAdapter, ToString)
TEST(RichEnum, Ordinal)
{
{
static_assert(TestRichEnum1::C_ONE().ordinal() == 0);
static_assert(TestRichEnum1::C_TWO().ordinal() == 1);
static_assert(TestRichEnum1::C_THREE().ordinal() == 2);
static_assert(TestRichEnum1::C_FOUR().ordinal() == 3);
static_assert(TestRichEnum1::C_ONE->ordinal() == 0);
static_assert(TestRichEnum1::C_TWO->ordinal() == 1);
static_assert(TestRichEnum1::C_THREE->ordinal() == 2);
static_assert(TestRichEnum1::C_FOUR->ordinal() == 3);
}

{
static_assert(TestRichEnum2::C_ONE().ordinal() == 0);
static_assert(TestRichEnum2::C_TWO().ordinal() == 1);
static_assert(TestRichEnum2::C_THREE().ordinal() == 2);
static_assert(TestRichEnum2::C_FOUR().ordinal() == 3);
static_assert(TestRichEnum2::C_ONE->ordinal() == 0);
static_assert(TestRichEnum2::C_TWO->ordinal() == 1);
static_assert(TestRichEnum2::C_THREE->ordinal() == 2);
static_assert(TestRichEnum2::C_FOUR->ordinal() == 3);
}
}

TEST(RichEnum, ValueOfName)
{
{
static_assert(TestRichEnum1::value_of("C_ONE") == TestRichEnum1::C_ONE());
static_assert(TestRichEnum1::value_of("C_TWO") == TestRichEnum1::C_TWO());
static_assert(TestRichEnum1::value_of("C_THREE") == TestRichEnum1::C_THREE());
static_assert(TestRichEnum1::value_of("C_FOUR") == TestRichEnum1::C_FOUR());
static_assert(TestRichEnum1::value_of("C_ONE") == TestRichEnum1::C_ONE);
static_assert(TestRichEnum1::value_of("C_TWO") == TestRichEnum1::C_TWO);
static_assert(TestRichEnum1::value_of("C_THREE") == TestRichEnum1::C_THREE);
static_assert(TestRichEnum1::value_of("C_FOUR") == TestRichEnum1::C_FOUR);
static_assert(TestRichEnum1::value_of("INVALID") == std::nullopt);
}

{
constexpr const TestRichEnum1& MY_VALUE = TestRichEnum1::value_of("C_ONE").value();

static_assert(MY_VALUE == TestRichEnum1::C_ONE());
static_assert(&MY_VALUE == &TestRichEnum1::C_ONE());
static_assert(MY_VALUE == TestRichEnum1::C_ONE);
static_assert(&MY_VALUE == &TestRichEnum1::C_ONE);
}
}

TEST(RichEnum, ValueOfBackingEnum)
{
{
using BE = detail::TestRichEnum1BackingEnum;
static_assert(TestRichEnum1::value_of(BE::C_ONE) == TestRichEnum1::C_ONE());
static_assert(TestRichEnum1::value_of(BE::C_TWO) == TestRichEnum1::C_TWO());
static_assert(TestRichEnum1::value_of(BE::C_THREE) == TestRichEnum1::C_THREE());
static_assert(TestRichEnum1::value_of(BE::C_FOUR) == TestRichEnum1::C_FOUR());
static_assert(TestRichEnum1::value_of(BE::C_ONE) == TestRichEnum1::C_ONE);
static_assert(TestRichEnum1::value_of(BE::C_TWO) == TestRichEnum1::C_TWO);
static_assert(TestRichEnum1::value_of(BE::C_THREE) == TestRichEnum1::C_THREE);
static_assert(TestRichEnum1::value_of(BE::C_FOUR) == TestRichEnum1::C_FOUR);
static_assert(TestRichEnum1::value_of(static_cast<BE>(29)) == std::nullopt);
}

{
using BE = detail::TestRichEnum1BackingEnum;
constexpr const TestRichEnum1& MY_VALUE = TestRichEnum1::value_of(BE::C_ONE).value();

static_assert(MY_VALUE == TestRichEnum1::C_ONE());
static_assert(&MY_VALUE == &TestRichEnum1::C_ONE());
static_assert(MY_VALUE == TestRichEnum1::C_ONE);
static_assert(&MY_VALUE == &TestRichEnum1::C_ONE);
}
}

TEST(RichEnum, ValueOfUnderlyingInt)
{
{
static_assert(TestRichEnum1::value_of(19) == TestRichEnum1::C_ONE());
static_assert(TestRichEnum1::value_of(21) == TestRichEnum1::C_TWO());
static_assert(TestRichEnum1::value_of(23) == TestRichEnum1::C_THREE());
static_assert(TestRichEnum1::value_of(25) == TestRichEnum1::C_FOUR());
static_assert(TestRichEnum1::value_of(19) == TestRichEnum1::C_ONE);
static_assert(TestRichEnum1::value_of(21) == TestRichEnum1::C_TWO);
static_assert(TestRichEnum1::value_of(23) == TestRichEnum1::C_THREE);
static_assert(TestRichEnum1::value_of(25) == TestRichEnum1::C_FOUR);
static_assert(TestRichEnum1::value_of(29) == std::nullopt);
}

{
constexpr const TestRichEnum1& MY_VALUE = TestRichEnum1::value_of(19).value();

static_assert(MY_VALUE == TestRichEnum1::C_ONE());
static_assert(&MY_VALUE == &TestRichEnum1::C_ONE());
static_assert(MY_VALUE == TestRichEnum1::C_ONE);
static_assert(&MY_VALUE == &TestRichEnum1::C_ONE);
}
}

Expand All @@ -292,23 +292,23 @@ TEST(RichEnum, HasValue)
constexpr TestRichEnum1 INVALID{};
static_assert(!INVALID.has_value());
static_assert(INVALID == TestRichEnum1{});
static_assert(INVALID != TestRichEnum1::C_ONE());
static_assert(INVALID != TestRichEnum1::C_TWO());
static_assert(INVALID != TestRichEnum1::C_THREE());
static_assert(INVALID != TestRichEnum1::C_FOUR());
static_assert(INVALID != TestRichEnum1::C_ONE);
static_assert(INVALID != TestRichEnum1::C_TWO);
static_assert(INVALID != TestRichEnum1::C_THREE);
static_assert(INVALID != TestRichEnum1::C_FOUR);
}

TEST(RichEnum, BoolNegate)
{
{
constexpr const TestRichEnumBool& F_VALUE = TestRichEnumBool::FALSE_VALUE();
constexpr const TestRichEnumBool& F_VALUE = TestRichEnumBool::FALSE_VALUE;
static_assert(F_VALUE.has_value());
static_assert((!F_VALUE) == TestRichEnumBool::TRUE_VALUE());
static_assert((!F_VALUE) == TestRichEnumBool::TRUE_VALUE);
}
{
constexpr const TestRichEnumBool& T_VALUE = TestRichEnumBool::TRUE_VALUE();
constexpr const TestRichEnumBool& T_VALUE = TestRichEnumBool::TRUE_VALUE;
static_assert(T_VALUE.has_value());
static_assert((!T_VALUE) == TestRichEnumBool::FALSE_VALUE());
static_assert((!T_VALUE) == TestRichEnumBool::FALSE_VALUE);
}
}

Expand All @@ -329,8 +329,8 @@ constexpr void rich_enum_constants_can_be_used_as_a_template_parameter()

TEST(RichEnum, UsageAsTemplateParameter)
{
rich_enum_constants_can_be_used_as_a_template_parameter<TestRichEnum1::C_TWO()>();
const RichEnumConstantsCanBeUsedAsATemplateParameter<TestRichEnum1::C_TWO()> my_struct{};
rich_enum_constants_can_be_used_as_a_template_parameter<TestRichEnum1::C_TWO>();
const RichEnumConstantsCanBeUsedAsATemplateParameter<TestRichEnum1::C_TWO> my_struct{};
static_cast<void>(my_struct);
}

Expand All @@ -340,16 +340,16 @@ TEST(RichEnum, UsageInSwitchCase)
{
switch (val)
{
case TestRichEnum1::C_ONE():
case TestRichEnum1::C_ONE:
return 11;
case TestRichEnum1::C_TWO():
case TestRichEnum1::C_TWO:
return 22;
case TestRichEnum1::C_THREE():
case TestRichEnum1::C_THREE:
return 33;
case TestRichEnum1::C_FOUR():
case TestRichEnum1::C_FOUR:
return 44;
}
}(TestRichEnum1::C_TWO());
}(TestRichEnum1::C_TWO);

static_assert(22 == result);
}
Expand Down
Loading