Skip to content

Matching Python 2 int behavior on Python 2 #1186

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

Merged
merged 2 commits into from
Nov 30, 2017
Merged

Conversation

henryiii
Copy link
Collaborator

This is from a Gitter discussion. Pybind11's default conversion to int always produces a long on Python 2 (ints and longs were unified in Python 3). This patch fixes int handling to match Python 2 on Python 2; for short types (size_t or smaller), the number will be returned as an int if possible, otherwise long. Requires Python 2.5+.

An example of where this matters: The sys.exit function must have an int. So if you have this:

.def("__int__", &Thing::operator int)

You currently need this to work;

sys.exit(int(int(thing)))

My Python 2 numpy is having issues, so can't test locally yet.

@henryiii
Copy link
Collaborator Author

I believe this works now, but Clang 5.0 is broken for everyone right now. Someone will need to restart it tomorrow.



def test_int_long():
"""In Python 2, a C++ int should return a Python int if possible, not long."""
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps elaborate a bit on why:

In Python 2, a C++ int should return a Python int rather than long if possible: int's can always be used where a long is required, but longs are not always accepted where ints are used (such as the argument to sys.exit()).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is possible (though extremely rare) that a long could be required; you could just check the type, for example. But now you can force a long in C++ by casting to long long.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated.

@@ -980,11 +980,19 @@ struct type_caster<T, enable_if_t<std::is_arithmetic<T>::value && !is_std_char_t
static handle cast(T src, return_value_policy /* policy */, handle /* parent */) {
if (std::is_floating_point<T>::value) {
return PyFloat_FromDouble((double) src);
} else if (sizeof(T) <= sizeof(long)) {
} else if (sizeof(T) <= sizeof(ssize_t)) {
#if PY_VERSION_HEX < 0x03000000
Copy link
Member

Choose a reason for hiding this comment

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

Instead of the #ifdef here, let's add these to detail/common.h like we have for various other python API calls that differ across python versions:

#define PYBIND11_LONG_FROM_SIGNED(o) PyLong_FromSize_t((size_t) o)
#define PYBIND11_LONG_FROM_UNSIGNED(o) PyLong_FromSsize_t((ssize_t) o)

for Python 3 and

#define PYBIND11_LONG_FROM_SIGNED(o) PyInt_FromSize_t((size_t) o)     // Returns long if needed.
#define PYBIND11_LONG_FROM_UNSIGNED(o) PyInt_FromSsize_t((ssize_t) o) // Likewise.

for Python 2; then this part of the cast.h code becomes much cleaner:

            if (std::is_signed<T>::value)
                return PYBIND11_LONG_FROM_SIGNED(o);
            else
                PYBIND11_LONG_FROM_UNSIGNED(o);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I was considering something like that. I'll add when the CI build finishes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@henryiii
Copy link
Collaborator Author

Repushed, now that Clang 5 is working again.

@wjakob
Copy link
Member

wjakob commented Nov 16, 2017

This looks great.


import sys
must_be_long = type(getattr(sys, 'maxint', 1) + 1)
isinstance(m.int_cast(), int)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't these be assert isinstance(...)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops!

@jagerman
Copy link
Member

Aside from the missing asserts in the test, this looks good.

@jagerman
Copy link
Member

Looks like apt.llvm.org is back to being broken, but more importantly, the added tests don't appear to be passing under Python 2.

@henryiii
Copy link
Collaborator Author

henryiii commented Nov 16, 2017

The problem is long long == ssize_t. Which is nice, that means that everything is covered on most systems though just that one Python API call. The current method of a backup call is still best just in case size_t is 32 bits, such as on some 32 bit systems.

Python ints are signed, so the max unsigned long long should trigger a python long return.

@jagerman
Copy link
Member

The i386 build should hit it; I think there long long is 64 bits but size_t is 32.

@henryiii
Copy link
Collaborator Author

Can this be merged? I think it is finished from my end.

@jagerman jagerman merged commit cf0d0f9 into pybind:master Nov 30, 2017
@jagerman
Copy link
Member

Yeah, thanks for the reminder (I'm pretty swamped with other things at the moment).

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.

3 participants