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

[zhangyangyu]

BPO	27660
Nosy	@tim-one, @rhettinger, @mdickinson, @vstinner, @bitdancer, @vadmium, @serhiy-storchaka, @zhangyangyu, @Mariatta
PRs	bpo-27660: remove unnecessary overflow checks in list_resize #189
Files	list_resize.patch list_resize_v2.patch list_resize_v3.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/zhangyangyu'
closed_at = <Date 2017-02-22.04:32:53.486>
created_at = <Date 2016-07-31.17:40:09.746>
labels = ['interpreter-core', 'type-feature', '3.7']
title = 'Replace size_t with Py_ssize_t as the type of local variable in list_resize'
updated_at = <Date 2017-02-22.04:32:53.485>
user = 'https://github.com/zhangyangyu'

bugs.python.org fields:

activity = <Date 2017-02-22.04:32:53.485>
actor = 'xiang.zhang'
assignee = 'xiang.zhang'
closed = True
closed_date = <Date 2017-02-22.04:32:53.486>
closer = 'xiang.zhang'
components = ['Interpreter Core']
creation = <Date 2016-07-31.17:40:09.746>
creator = 'xiang.zhang'
dependencies = []
files = ['43957', '43976', '44192']
hgrepos = []
issue_num = 27660
keywords = ['patch']
message_count = 22.0
messages = ['271746', '271748', '271749', '271765', '271784', '271785', '271799', '271806', '271808', '271827', '271829', '273378', '273398', '273401', '273418', '288187', '288256', '288260', '288264', '288266', '288268', '288328']
nosy_count = 9.0
nosy_names = ['tim.peters', 'rhettinger', 'mark.dickinson', 'vstinner', 'r.david.murray', 'martin.panter', 'serhiy.storchaka', 'xiang.zhang', 'Mariatta']
pr_nums = ['189']
priority = 'low'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue27660'
versions = ['Python 3.7']

[zhangyangyu]

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 second overflow check.

[bitdancer]

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.

[bitdancer]

Oh, I got that backward didn't I. My rusty C skills :(

[zhangyangyu]

:( Sorry David, my poor English doesn't enable me understand your message totally. Is list.sort related to this problem? Do I miss something?

[bitdancer]

No, I was the one who missed something. Just ignore my comment and lets wait for someone with more current C chops to answer :)

[bitdancer]

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.

[vadmium]

The patch looks okay to me.

[serhiy-storchaka]

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 -1;
    }

Checking "new_allocated < (size_t)newsize" can be more efficient than "new_allocated > PY_SSIZE_T_MAX - newsize".

[zhangyangyu]

But if you use size_t, you are checking overflow against PY_SIZE_MAX, not PY_SSIZE_T_MAX.

[serhiy-storchaka]

True. Then we should use the condition "new_allocated > PY_SSIZE_T_MAX". This can be even more efficient.

[zhangyangyu]

So how about list_resize_v2.patch?

[zhangyangyu]

Ping.