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-39200: Correct the error message for range() empty constructor #17813
Conversation
|
For me, this is more correct :) |
|
Would be nice to add tests. |
| @@ -78,8 +78,12 @@ range_new(PyTypeObject *type, PyObject *args, PyObject *kw) | |||
| return NULL; | |||
|
|
|||
| if (PyTuple_Size(args) <= 1) { | |||
| if (!PyArg_UnpackTuple(args, "range", 1, 1, &stop)) | |||
| if (!PyArg_UnpackTuple(args, "range", 1, 1, &stop)) { | |||
This comment has been minimized.
This comment has been minimized.
serhiy-storchaka
Jan 3, 2020
•
Member
I think it is better to get rid of PyArg_UnpackTuple(). Its only benefit is checking the size of the tuple and formatting an error message which is wrong. Note also that the second PyArg_UnpackTuple() also can produce incorrect error message.
switch (PyTuple_GET_SIZE(args)) {
case 3:
step = PyTuple_GET_ITEM(args, 2);
/* fall through */
case 2:
start = PyTuple_GET_ITEM(args, 0);
stop = PyTuple_GET_ITEM(args, 1);
...
break;
case 1:
stop = PyTuple_GET_ITEM(args, 0);
...
break;
case 0:
// error
return NULL;
default:
// error
return NULL;
}
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Other option - just use |
This comment has been minimized.
This comment has been minimized.
I have implemented the switch statement as we would need to do the dance with |
This comment has been minimized.
This comment has been minimized.
Done in eb9e784 |
This comment has been minimized.
This comment has been minimized.
|
Btw, as a bonus the constructor of master./python.exe -m pyperf timeit "range(1,3,4)" patched./python.exe -m pyperf timeit "range(1,3,4)" |
pablogsal commentedJan 3, 2020
•
edited by bedevere-bot
https://bugs.python.org/issue39200