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.

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

Fix enum value's __int__ returning non-int when underlying type is bool or of char type #1334

Merged
merged 8 commits into from
Aug 26, 2021
21 changes: 19 additions & 2 deletions include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -1814,6 +1814,19 @@ struct enum_base {
handle m_parent;
};

template <bool is_signed, size_t length> struct equivalent_integer {};
template <> struct equivalent_integer<true, 1> { using type = int8_t; };
template <> struct equivalent_integer<false, 1> { using type = uint8_t; };
template <> struct equivalent_integer<true, 2> { using type = int16_t; };
template <> struct equivalent_integer<false, 2> { using type = uint16_t; };
template <> struct equivalent_integer<true, 4> { using type = int32_t; };
template <> struct equivalent_integer<false, 4> { using type = uint32_t; };
template <> struct equivalent_integer<true, 8> { using type = int64_t; };
template <> struct equivalent_integer<false, 8> { using type = uint64_t; };

template <typename IntLike>
using equivalent_integer_t = typename equivalent_integer<std::is_signed<IntLike>::value, sizeof(IntLike)>::type;

PYBIND11_NAMESPACE_END(detail)

/// Binds C++ enumerations and enumeration classes to Python
Expand All @@ -1824,13 +1837,17 @@ template <typename Type> class enum_ : public class_<Type> {
using Base::attr;
using Base::def_property_readonly;
using Base::def_property_readonly_static;
using Scalar = typename std::underlying_type<Type>::type;
using Underlying = typename std::underlying_type<Type>::type;
// Scalar is the integer representation of underlying type
using Scalar = detail::conditional_t<detail::any_of<
detail::is_std_char_type<Underlying>, std::is_same<Underlying, bool>
>::value, detail::equivalent_integer_t<Underlying>, Underlying>;

template <typename... Extra>
enum_(const handle &scope, const char *name, const Extra&... extra)
: class_<Type>(scope, name, extra...), m_base(*this, scope) {
constexpr bool is_arithmetic = detail::any_of<std::is_same<arithmetic, Extra>...>::value;
constexpr bool is_convertible = std::is_convertible<Type, Scalar>::value;
constexpr bool is_convertible = std::is_convertible<Type, Underlying>::value;
m_base.init(is_arithmetic, is_convertible);

def(init([](Scalar i) { return static_cast<Type>(i); }), arg("value"));
Expand Down
61 changes: 61 additions & 0 deletions tests/test_enum.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,65 @@ TEST_SUBMODULE(enums, m) {
.value("ONE", SimpleEnum::THREE)
.export_values();
});

// test_enum_scalar
enum UnscopedUCharEnum : unsigned char {};
enum class ScopedShortEnum : short {};
enum class ScopedLongEnum : long {};
enum UnscopedUInt64Enum : std::uint64_t {};
static_assert(py::detail::all_of<
std::is_same<py::enum_<UnscopedUCharEnum>::Scalar, unsigned char>,
std::is_same<py::enum_<ScopedShortEnum>::Scalar, short>,
std::is_same<py::enum_<ScopedLongEnum>::Scalar, long>,
std::is_same<py::enum_<UnscopedUInt64Enum>::Scalar, std::uint64_t>
>::value, "Error during the deduction of enum's scalar type with normal integer underlying");

// test_enum_scalar_with_char_underlying
enum class ScopedCharEnum : char { Zero, Positive };
enum class ScopedWCharEnum : wchar_t { Zero, Positive };
enum class ScopedChar32Enum : char32_t { Zero, Positive };
enum class ScopedChar16Enum : char16_t { Zero, Positive };

// test the scalar of char type enums according to chapter 'Character types'
// from https://en.cppreference.com/w/cpp/language/types
static_assert(py::detail::any_of<
std::is_same<py::enum_<ScopedCharEnum>::Scalar, signed char>, // e.g. gcc on x86
std::is_same<py::enum_<ScopedCharEnum>::Scalar, unsigned char> // e.g. arm linux
>::value, "char should be cast to either signed char or unsigned char");
static_assert(
sizeof(py::enum_<ScopedWCharEnum>::Scalar) == 2 ||
sizeof(py::enum_<ScopedWCharEnum>::Scalar) == 4
, "wchar_t should be either 16 bits (Windows) or 32 (everywhere else)");
static_assert(py::detail::all_of<
std::is_same<py::enum_<ScopedChar32Enum>::Scalar, std::uint_least32_t>,
std::is_same<py::enum_<ScopedChar16Enum>::Scalar, std::uint_least16_t>
>::value, "char32_t, char16_t (and char8_t)'s size, signedness, and alignment is determined");
#if defined(PYBIND11_CPP20)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a nit: we should probably start testing the CPP20 soon.

Copy link
Contributor Author

@Vigilans Vigilans Aug 26, 2021

Choose a reason for hiding this comment

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

Thanks for your rebase and review! And sorry for my long delay on last comment since I didn't check github's email frequently at that time...

What should I do to help the testing of CPP20 in this pull request? As shown in detail::is_std_char_type's implementation:

template <typename CharT> using is_std_char_type = any_of<
std::is_same<CharT, char>, /* std::string */
#if defined(PYBIND11_HAS_U8STRING)
std::is_same<CharT, char8_t>, /* std::u8string */
#endif
std::is_same<CharT, char16_t>, /* std::u16string */
std::is_same<CharT, char32_t>, /* std::u32string */
std::is_same<CharT, wchar_t> /* std::wstring */
>;

Should I replace the PYBIND11_CPP20 macro with PYBIND11_HAS_U8STRING in this test?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What should I do to help the testing of CPP20 in this pull request?

The first step is on us (Aaron, Henry, me), we need to add a job to the CI.
Not sure if we should block this PR on that, or merge this PR first and then fix up as needed when establishing the CPP20 job. I'm fine either way.

enum class ScopedChar8Enum : char8_t { Zero, Positive };
static_assert(std::is_same<py::enum_<ScopedChar8Enum>::Scalar, unsigned char>::value);
#endif

// test_char_underlying_enum
py::enum_<ScopedCharEnum>(m, "ScopedCharEnum")
.value("Zero", ScopedCharEnum::Zero)
.value("Positive", ScopedCharEnum::Positive);
py::enum_<ScopedWCharEnum>(m, "ScopedWCharEnum")
.value("Zero", ScopedWCharEnum::Zero)
.value("Positive", ScopedWCharEnum::Positive);
py::enum_<ScopedChar32Enum>(m, "ScopedChar32Enum")
.value("Zero", ScopedChar32Enum::Zero)
.value("Positive", ScopedChar32Enum::Positive);
py::enum_<ScopedChar16Enum>(m, "ScopedChar16Enum")
.value("Zero", ScopedChar16Enum::Zero)
.value("Positive", ScopedChar16Enum::Positive);

// test_bool_underlying_enum
enum class ScopedBoolEnum : bool { FALSE, TRUE };

// bool is unsigned (std::is_signed returns false) and 1-bit long, so represented with u8
static_assert(std::is_same<py::enum_<ScopedBoolEnum>::Scalar, std::uint8_t>::value, "");

py::enum_<ScopedBoolEnum>(m, "ScopedBoolEnum")
.value("FALSE", ScopedBoolEnum::FALSE)
.value("TRUE", ScopedBoolEnum::TRUE);
}
24 changes: 24 additions & 0 deletions tests/test_enum.py
Original file line number Diff line number Diff line change
Expand Up @@ -218,10 +218,16 @@ def test_binary_operators():
def test_enum_to_int():
m.test_enum_to_int(m.Flags.Read)
m.test_enum_to_int(m.ClassWithUnscopedEnum.EMode.EFirstMode)
m.test_enum_to_int(m.ScopedCharEnum.Positive)
m.test_enum_to_int(m.ScopedBoolEnum.TRUE)
m.test_enum_to_uint(m.Flags.Read)
m.test_enum_to_uint(m.ClassWithUnscopedEnum.EMode.EFirstMode)
m.test_enum_to_uint(m.ScopedCharEnum.Positive)
m.test_enum_to_uint(m.ScopedBoolEnum.TRUE)
m.test_enum_to_long_long(m.Flags.Read)
m.test_enum_to_long_long(m.ClassWithUnscopedEnum.EMode.EFirstMode)
m.test_enum_to_long_long(m.ScopedCharEnum.Positive)
m.test_enum_to_long_long(m.ScopedBoolEnum.TRUE)


def test_duplicate_enum_name():
Expand All @@ -230,6 +236,24 @@ def test_duplicate_enum_name():
assert str(excinfo.value) == 'SimpleEnum: element "ONE" already exists!'


def test_char_underlying_enum():
assert type(m.ScopedCharEnum.Positive.__int__()) == int
Vigilans marked this conversation as resolved.
Show resolved Hide resolved
assert m.ScopedCharEnum.Zero.__int__() == 0
assert m.ScopedChar32Enum.Positive.__getstate__() == 1
assert m.ScopedChar32Enum.Zero.__hash__() == 0
int(m.ScopedChar16Enum.Positive)
Vigilans marked this conversation as resolved.
Show resolved Hide resolved
hash(m.ScopedChar16Enum.Zero)
m.ScopedWCharEnum(1)
with pytest.raises(TypeError):
m.ScopedWCharEnum("0")


def test_bool_underlying_enum():
assert type(m.ScopedBoolEnum.TRUE.__int__()) == int
assert m.ScopedBoolEnum.FALSE.__hash__() == 0
m.ScopedBoolEnum(1)


def test_docstring_signatures():
for enum_type in [m.ScopedEnum, m.UnscopedEnum]:
for attr in enum_type.__dict__.values():
Expand Down