[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Xiang Zhang added the comment: New changeset 4cee049f5b6864066b8315e9b54de955e5487dfc by GitHub in branch 'master': bpo-27660: remove unnecessary overflow checks in list_resize (GH-189) https://github.com/python/cpython/commit/4cee049f5b6864066b8315e9b54de955e5487dfc --

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Changes by Xiang Zhang : -- resolution: -> fixed stage: commit review -> resolved status: open -> closed ___ Python tracker ___

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Raymond Hettinger added the comment: Okay, go ahead. I was hoping to give Mariatta some practice applying C patches (she's new). -- ___ Python tracker

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Xiang Zhang added the comment: Sorry, I have the commit bit and know what to do with a commit. So I assign it to myself and wait someone approve the PR on GitHub. -- assignee: Mariatta -> xiang.zhang stage: patch review -> commit review ___ Python

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Raymond Hettinger added the comment: Mariatta, would you like to run this against the test suite and then apply it. I don't think a MISC/NEWS entry is needed (it is just a code simplification) and Xiang is already in MISC/ACKS. -- assignee: rhettinger -> Mariatta nosy: +Mariatta

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Xiang Zhang added the comment: Thanks for your time Raymond. :-) I applied your suggestions in the PR. -- ___ Python tracker ___

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Raymond Hettinger added the comment: It was a little mind-numbing to check this code, but it looks correct. Please fix two nits for better readability. For review purposes, it was nice to have changes stand-out, but for the final code I would like to combine the two comments and the two

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Xiang Zhang added the comment: I opened a PR on GitHub for this issue. Hope Raymond you could review it some time. -- versions: +Python 3.7 -Python 3.6 ___ Python tracker

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Changes by Xiang Zhang : -- pull_requests: +155 ___ Python tracker ___ ___ Python-bugs-list

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Xiang Zhang added the comment: v3 applies haypo's suggestion. -- Added file: http://bugs.python.org/file44192/list_resize_v3.patch ___ Python tracker ___

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

STINNER Victor added the comment: I reviewed list_resize_v2.patch. -- nosy: +haypo ___ Python tracker ___ ___

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Raymond Hettinger added the comment: This is low priority. I will look at it during the sprint. As far as I know there is no bug here or performance problem; instead, the patch alters stable code and transfers responsibilities. -- priority: normal -> low

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Xiang Zhang added the comment: Ping. -- ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe:

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Xiang Zhang added the comment: So how about list_resize_v2.patch? -- Added file: http://bugs.python.org/file43976/list_resize_v2.patch ___ Python tracker

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Serhiy Storchaka added the comment: True. Then we should use the condition "new_allocated > PY_SSIZE_T_MAX". This can be even more efficient. -- ___ Python tracker

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Xiang Zhang added the comment: But if you use size_t, you are checking overflow against PY_SIZE_MAX, not PY_SSIZE_T_MAX. -- ___ Python tracker ___

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Serhiy Storchaka added the comment: Unsigned type can be used for more efficient checking on integer overflow. new_allocated = (newsize >> 3) + (newsize < 9 ? 3 : 6); new_allocated += (size_t)newsize; if (new_allocated < (size_t)newsize) { PyErr_NoMemory(); return

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Changes by Raymond Hettinger : -- nosy: +mark.dickinson ___ Python tracker ___

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Changes by Raymond Hettinger : -- assignee: -> rhettinger nosy: +rhettinger, serhiy.storchaka, tim.peters ___ Python tracker

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Martin Panter added the comment: The patch looks okay to me. -- stage: -> patch review ___ Python tracker ___

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

R. David Murray added the comment: Erg. You mention your English proficiency and then I go using a colequialism :( "better C chops" means someone with more skill than I am currently exhibiting. -- ___ Python tracker

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

R. David Murray added the comment: No, I was the one who missed something. Just ignore my comment and lets wait for someone with more current C chops to answer :) -- ___ Python tracker

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

Xiang Zhang added the comment: :( Sorry David, my poor English doesn't enable me understand your message totally. Is list.sort related to this problem? Do I miss something? -- ___ Python tracker

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

R. David Murray added the comment: Oh, I got that backward didn't I. My rusty C skills :( -- ___ Python tracker ___

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

R. David Murray added the comment: ob->allocated is temporarily set to -1 by list.sort to detect mutations, so the restriction of new_allocated to size_t is probably intentional. If so, though, there ought to be a comment making that clear. -- nosy: +r.david.murray

[issue27660] Replace size_t with Py_ssize_t as the type of local variable in list_resize

New submission from Xiang Zhang: In list_resize, new_allocated is of type size_t but I think don't have to be now since it finally have to assign back to self->allocated which is of type Py_ssize_t. With Py_ssize_t, we can check some overflows in the first overflow check and don't need the