Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upbpo-37207: Use vectorcall for range() #18464
Conversation
| range_vectorcall(PyTypeObject *type, PyObject *const *args, | ||
| size_t nargsf, PyObject *kwnames) | ||
| { | ||
| size_t nargs = PyVectorcall_NARGS(nargsf); |
This comment has been minimized.
This comment has been minimized.
vstinner
Feb 11, 2020
Member
Oh, I didn't expect a size_t here. I opened an issue to discuss PyVectorcall_NARGS() return type: https://bugs.python.org/issue39611
This comment has been minimized.
This comment has been minimized.
encukou
Feb 18, 2020
•
Author
Member
The return type already is Py_ssize_t. The bug was only in this patch.
(When Mark was originally writing the patch, there was a discussion of Py_ssize_t vs. size_t. Mark is a big proponent of the unsigned type, which is more correct. But also much less compatible with the existing codebase.)
| @@ -71,43 +72,35 @@ make_range_object(PyTypeObject *type, PyObject *start, | |||
| range(0, 5, -1) | |||
| */ | |||
| static PyObject * | |||
| range_new(PyTypeObject *type, PyObject *args, PyObject *kw) | |||
| range_from_array(PyTypeObject *type, PyObject *const *args, size_t num_args) | |||
This comment has been minimized.
This comment has been minimized.
vstinner
Feb 11, 2020
Member
I would prefer to use Py_ssize_t type for num_args. Usually, it's called "nargs", but I'm fine with "num_args" ;-)
This comment has been minimized.
This comment has been minimized.
encukou
Feb 18, 2020
Author
Member
Right, Py_ssize_t is more consistent.
But I find num_args more descriptive, so I'll keep that name.
This comment has been minimized.
This comment has been minimized.
codecov
bot
commented
Feb 11, 2020
•
Codecov Report
@@ Coverage Diff @@
## master #18464 +/- ##
===========================================
+ Coverage 82.11% 83.20% +1.08%
===========================================
Files 1955 1571 -384
Lines 588958 414749 -174209
Branches 44428 44456 +28
===========================================
- Hits 483632 345077 -138555
+ Misses 95677 60024 -35653
+ Partials 9649 9648 -1
Continue to review full report at Codecov.
|
| @@ -122,14 +115,14 @@ range_new(PyTypeObject *type, PyObject *args, PyObject *kw) | |||
| return NULL; | |||
| default: | |||
| PyErr_Format(PyExc_TypeError, | |||
| "range expected at most 3 arguments, got %zd", | |||
| "range expected at most 3 arguments, got %zu", | |||
| num_args); | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
LGTM. |
6e35da9
into
python:master
This continues the `range()` part of python#13930. The complete pull request is stalled on discussions around dicts, but `range()` should not be controversial. (And I plan to open PRs for other parts if this is merged.) On top of Mark's change, I unified `range_new` and `range_vectorcall`, which had a lot of duplicate code. https://bugs.python.org/issue37207
This continues the `range()` part of python#13930. The complete pull request is stalled on discussions around dicts, but `range()` should not be controversial. (And I plan to open PRs for other parts if this is merged.) On top of Mark's change, I unified `range_new` and `range_vectorcall`, which had a lot of duplicate code. https://bugs.python.org/issue37207
encukou commentedFeb 11, 2020
•
edited by miss-islington
This continues the
range()part of #13930. The complete pull request is stalled on discussions around dicts, butrange()should not be controversial. (And I plan to open PRs for other parts if this is merged.)On top of Mark's change, I unified
range_newandrange_vectorcall, which had a lot of duplicate code.https://bugs.python.org/issue37207
Automerge-Triggered-By: @encukou