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

[Vigilans]

Introduction

This PR solves the problem of enum value's __int__ returning a non-int value when its underlying type is bool or of char type.

Detail

This PR extends the enum_ class with following type alias:

• Underlying type: the actual underlying type of enum (The original Scalar type);
• Scalar type: the integer type with length and signedness equivalent to the underlying type.

The enum should always use Scalar as the inner integer type when dealing with python side.

So,

• Scalar for underlying type of normal integer would stay the same;
• Scalar for underlying type of bool or char type would use the int type of same length and signedness, specified through the equivalent_integer struct.

Applicable issues

Fix #1331
Fix #1820
Fix osmcode/pyosmium#114

Suggested changelog entry:

Bug fix: enum value's `__int__` returning non-int when underlying type is bool or of char type. Fixes #1331, #1820.

[jagerman]

Is unsigned char (or, for that matter, signed char, which is also distinct from char and unsigned char) necessary here? That is_std_char_type template is there to capture the types that are used for strings, which is pretty much just char. It shouldn't be catching unsigned char and signed char in the first place--those should fall through to the arithmetic converter.

[Vigilans]

@jagerman

I see. It corresponds to the compile error which seems to tell me that unsigned char is used as an arithmetic type, instead of an element of string, which is listed inside the is_std_char_type.

[Vigilans]

Sorry for my foolishness of not getting your point and suspended this PR for one year long...

I've updated the PR to keep pace with the master, and fixed it with a more robust mechanism, along with more tests, to solve both issues of char and bool types.

My original post for context reference:

I followed the suggestion mentioned in the issue and now the bug is eliminated.

BTW, It should be noted that the definition of detailed::is_std_char_type does not check unsigned char:

template <typename CharT> using is_std_char_type = any_of<
    std::is_same<CharT, char>, /* std::string */
    std::is_same<CharT, char16_t>, /* std::u16string */
    std::is_same<CharT, char32_t>, /* std::u32string */
    std::is_same<CharT, wchar_t> /* std::wstring */
>;

And when I tried to add unsigned char to the type list, compile errors occurred. Therefore, I alternatively turn to add it to the definition of enum_<Type>::ScalarInt:

using ScalarInt = detail::conditional_t<
    detail::is_std_char_type<Scalar>::value || std::is_same<Scalar, unsigned char>::value,
    detail::conditional_t<std::is_signed<Scalar>::value, int, unsigned>,
    Scalar>;

[Vigilans]

Hi @wjakob @jagerman, now CI has been fixed, would you please take some time to review it?

[ax3l]

HI @Vigilans - looking through unfixed problems with propose solutions, do you think it would be possible to rebase this pull request for a new round of review? Sorry for the long delay on this!

[Skylion007]

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

[Vigilans]

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:

pybind11/include/pybind11/cast.h

Lines 111 to 119 in 031a700

	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?

[rwgk]

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.

[rwgk]

Awesome, thanks! Could you please leave a small comment "ready to merge!" when it is. (I see you made a series of small changes, therefore I'm not sure if you're still working on something.)

[Vigilans]

@rwgk Thanks for your patience! I'm working on satisfying the formatter and fixing test failure in some python 2.7 platforms (__getstate__() and __hash__() returns long type instead of int in these py2.7 platforms). When all the CI passed I'll leave a ready comment here.

[Vigilans]

@rwgk Seems some of the workflows need further approval. Personally I think it is ready to merge with:

• PYBIND11_CPP20 macro changed to PYBIND11_HAS_U8STRING and tested on a char8_t available environment settings.
• Python side tests modified to be supposed to pass in both python 3.x and python 2.x environments.

[rwgk]

CI all green! Thanks @Vigilans and all reviewers. Merging.