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

Make int_::operator T() (for integer types) behave the same as the caster for integers #2802

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

YannickJadoul
Copy link
Collaborator

@YannickJadoul YannickJadoul commented Jan 17, 2021

Description

Currently only demonstrates #2786, but soon hopefully fixes it.

Closes #2786.

Suggested changelog entry:

Conversion errors are now correctly handled by `py::int_`'s integer conversion operator.

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Just a quick glance. Not a full review.

Comment on lines +312 to +315
m.def("implicitly_cast_to_int32", [](py::int_ value) { return int32_t{value}; });
m.def("implicitly_cast_to_uint32", [](py::int_ value) { return uint32_t{value}; });
m.def("implicitly_cast_to_int64", [](py::int_ value) { return int64_t{value}; });
m.def("implicitly_cast_to_uint64", [](py::int_ value) { return uint64_t{value}; });
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are all explicit casts. If you want implicit, use trailing return types and just return value;.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think I per se want implicit casts. They invoke the same code anyway, right?

using py_type = detail::py_int_type_for<T>;
auto value = detail::as_integer<py_type>(m_ptr);
if (PyErr_Occurred())
throw error_already_set();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this now throwing OverflowError instead of RuntimeError?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this is what's currently making tests fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Missing error checking for overflowing integer casts
2 participants