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.

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.

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

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

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

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.

The patch looks okay to me.

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".

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

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

So how about list_resize_v2.patch?

Ping.

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.

I reviewed list_resize_v2.patch.

v3 applies haypo's suggestion.

I opened a PR on GitHub for this issue. Hope Raymond you could review it some time.

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 new_allocated assignments into a single comment and single assignment:

  /* The growth pattern is:  0, 4, 8, 16, 25, 35, 46, 58, 72, 88, ...
     Note: new_allocated won't overflow because the largest possible value
           is PY_SSIZE_T_MAX * (9 / 8) + 6 which always fits in a size_t.
  */
 new_allocated = (size_t)newsize + (newsize >> 3) + (newsize < 9 ? 3 : 6);

Secondly, please rename the "size" variable to something like "num_allocated_bytes". It is confusing to have "newsize" mean the number of elements actually used while having "size" mean the number of bytes actually allocated (including the overallocation).

Thanks for your time Raymond. :-) I applied your suggestions in the PR.

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.

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.

Okay, go ahead. I was hoping to give Mariatta some practice applying C patches (she's new).

New changeset 4cee049 by GitHub in branch 'master':
bpo-27660: remove unnecessary overflow checks in list_resize (GH-189)
4cee049