New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
a suboptimal check in list_extend() #75330
Comments
in listobject.c, in case list_extend() receives an 'iterable' which isn't 'self'
// (*)
if (Py_SIZE(self) < self->allocated) {
list_resize(self, Py_SIZE(self));
} IMHO, the condition in (*) is mostly useless, for two reasons:
Thus, i believe we should either change the condition in (*) to (note that i ignored error cases, as ISTM they are quite irrelevant to the |
-0 The test "Py_SIZE(self) < self->allocated" is very cheap so I don't mind leaving it in to handle the rare cases (or to prevent future bugs if the list_resize logic ever changes). |
I'm concerned that is would provide near zero benefit but would set us up for future bugs by tightly coupling two ideas that I would like to be able to reason about separately. The logic for list.extend() makes a potential overallocation, fills the list, and fixes-up the overallocation if needed (I want that logic should all stay together). And list_resize() should have its own separate logic as well. For a course grained operation like extend(), I really don't mind having a code path that makes a relatively cheap test that only occasionally useful. So, thank you for the suggestion, but I'm going to decline on the grounds that 1) the current code is sometimes useful, 2) it is always cheap, 3) that removing it is odds with the principles of loose coupling and high cohesion, 4) it creates a risk of introducing unnoticed errors in the future the list_resize logic were to change, and 5) no one else has stepped forward to advocate this patch. |
thank you for the elaborate reply :) do you feel the same about changing the check to |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: