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

Always call PyNumber_Index when casting from Python to a C++ integral type, also pre-3.8 #2801

Merged
merged 8 commits into from
Jan 25, 2021
Merged

Always call PyNumber_Index when casting from Python to a C++ integral type, also pre-3.8 #2801

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2231d9c
Always call PyNumber_Index when casting from Python to a C++ integral…
YannickJadoul Jan 16, 2021
2eb35ac
Fixed on PyPy
YannickJadoul Jan 17, 2021
064d671
Simplify use of PyNumber_Index, following @rwgk's idea, and ignore wa…
YannickJadoul Jan 20, 2021
caa5382
Reproduce mismatch between pre-3.8 and post-3.8 behavior on __index__…
YannickJadoul Jan 22, 2021
cffc586
Fix tests on 3.6 <= Python < 3.8
YannickJadoul Jan 22, 2021
8356073
No, I don't have an uninitialized variable
YannickJadoul Jan 22, 2021
2092295
Fix use of __index__ on Python 2
YannickJadoul Jan 23, 2021
7fd2db5
Make types in test_int_convert more ~boring~ descriptive
YannickJadoul Jan 25, 2021
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
31 changes: 25 additions & 6 deletions include/pybind11/cast.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1042,12 +1042,31 @@ struct type_caster<T, enable_if_t<std::is_arithmetic<T>::value && !is_std_char_t
return false;
} else if (!convert && !index_check(src.ptr()) && !PYBIND11_LONG_CHECK(src.ptr())) {
return false;
} else if (std::is_unsigned<py_type>::value) {
py_value = as_unsigned<py_type>(src.ptr());
} else { // signed integer:
py_value = sizeof(T) <= sizeof(long)
? (py_type) PyLong_AsLong(src.ptr())
: (py_type) PYBIND11_LONG_AS_LONGLONG(src.ptr());
} else {
handle obj = src;
YannickJadoul marked this conversation as resolved.
Show resolved Hide resolved
#if PY_VERSION_HEX < 0x03080000
bool do_decref = false;
YannickJadoul marked this conversation as resolved.
Show resolved Hide resolved
if (index_check(src.ptr())) {
PyObject *tmp = PyNumber_Index(src.ptr());
if (!tmp) {
PyErr_Clear();
return false;
}
do_decref = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

index_owner = reinterpret_steal<object>(tmp);

That way you don't need the second #if PY_VERSION_HEX < 0x03080000 below and this code become exception safe.

I'd also use idx (or similar) instead of tmp, to be more descriptive.

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 thought of/tried that, but didn't want to incur an overhead refcounting on Python >= 3.8, and this is also what CPython does.

But wait, maybe you mean something else, that doesn't need this! I'll give this a shot :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yesss, that does work out beautifully! Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Still seeing if this could easily be refactored out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, it's quite hard to refactor into a separate private function without incurring an additional inc_ref/dec_ref, it seems. It's already cleaner than before, though, so is it fine to leave like this for now?

obj = tmp;
}
#endif
if (std::is_unsigned<py_type>::value) {
py_value = as_unsigned<py_type>(obj.ptr());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This might seems like a reasonably big change, but after this PR, I want to fix #2786, which involves a minor refactoring of casting to C++ integer types (to ensure future consistency with py::int_::operator int()), so keep that in mind when reviewing, please ;-)

If we think that consistency between Python < 3.8 and >= 3.8 versions is a nice thing to have, then I personally don't really think this is a too high implementation price to pay.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(Also, #2786's fix shouldn't be complex either, so if you're able to wait 1 or 2 more days, it can also still be a fix to go into 2.6.2. But we need to draw a line somewhere, ofc.)

} else { // signed integer:
py_value = sizeof(T) <= sizeof(long)
? (py_type) PyLong_AsLong(obj.ptr())
: (py_type) PYBIND11_LONG_AS_LONGLONG(obj.ptr());
}
#if PY_VERSION_HEX < 0x03080000
if (do_decref)
obj.dec_ref();
#endif
}

// Python API reported an error
Expand Down
11 changes: 6 additions & 5 deletions tests/test_builtin_casters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,11 +289,12 @@ def cant_convert(v):
require_implicit(DeepThought())
cant_convert(ShallowThought())
cant_convert(FuzzyThought())
if env.PY >= (3, 8):
# Before Python 3.8, `int(obj)` does not pick up on `obj.__index__`
assert convert(IndexedThought()) == 42
assert noconvert(IndexedThought()) == 42
cant_convert(RaisingThought()) # no fall-back to `__int__`if `__index__` raises

# Before Python 3.8, `int(obj)` does not pick up on `obj.__index__`, but pybind11
# "backports" this behavior.
assert convert(IndexedThought()) == 42
assert noconvert(IndexedThought()) == 42
cant_convert(RaisingThought()) # no fall-back to `__int__`if `__index__` raises


def test_numpy_int_convert():
Expand Down