Skip to content

bpo-40792: Make PyNumber_Index() always returning exact int. #20443

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

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented May 27, 2020

@@ -112,6 +112,9 @@ The mathematical and bitwise operations are the most numerous:

Return *a* converted to an integer. Equivalent to ``a.__index__()``.

.. versionchanged:: 3.10
The type of the result is always :class:`int`.
Copy link
Member

@mdickinson mdickinson May 27, 2020

Choose a reason for hiding this comment

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

It may be worth adding a sentence to explain how this is different from the previous behaviour, since I think it'll come as a surprise to many that this wasn't already true before.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you please propose a wording? I have problems with formulating this in English.

Copy link
Member

Choose a reason for hiding this comment

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

So maybe something like:

The result always has exact type :class:int. Previously, the result could have been an instance of a subclass of int.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!

@@ -532,7 +532,7 @@ PyNumber_AsOff_t(PyObject *item, PyObject *err)
{
Py_off_t result;
PyObject *runerr;
PyObject *value = PyNumber_Index(item);
PyObject *value = _PyNumber_Index(item);
Copy link
Member

Choose a reason for hiding this comment

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

I'm missing something here: from my reading of the code, PyLong_AsOff_t is either PyLong_AsLong or PyLong_AsLongLong, and both of those already call _PyNumber_Index, so it looks as though we don't need this call.

Ah, I think I found what I'm missing: PyLong_AsOff_t could be PyLong_AsSsize_t, which doesn't call _PyNumber_Index. Is that right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is right.

It a separate question whether PyLong_AsSsize_t and others should call __index__ implicitly. It is not simple question, because such change may introduce bugs by making presumably atomic code non-atomic.

Copy link
Member

Choose a reason for hiding this comment

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

It a separate question whether PyLong_AsSsize_t and others should call __index__ implicitly.

Agreed. It seems worth looking into, but not in this PR.

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM in principle. (Modulo failing CI tests.)

_PyNumber_Index does feel a bit like a premature optimization. Though since the optimization is already present in the original behaviour of PyNumber_Index, I suppose it would be better described as a delayed pessimization.

I suspect we don't have very many cases where an integer-expecting function needs to be fast when passed an instance of an integer subclass - there just aren't that many interesting integer subclasses around, besides bool. (NumPy no longer has any.) I'd probably discourage use of _PyNumber_Index in new code, for simplicity, but it makes sense to use it as a replacement for PyNumber_Index in existing code.

@serhiy-storchaka
Copy link
Member Author

The "i" format unit is traditionally used to convert boolean arguments. Without such optimization we would need to create temporary integer objects 0 and 1 to convert boolean arguments. They are cached, but still.

Other case is using IntEnum. Many integer constants in the stdlib are replaced with IntEnum, but they are parsed as integers in the C code.

@serhiy-storchaka serhiy-storchaka merged commit 5f4b229 into python:master May 28, 2020
@serhiy-storchaka serhiy-storchaka deleted the pynumber-index-exact-int branch May 28, 2020 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants