bpo-36218: Fix handling of heterogeneous values in list.sort#12209
Conversation
|
Please add a NEWS entry since this fixes a segfault in a released version. |
Sorry, something went wrong.
|
Author of the bug here: apologies for the error, this patch is absolutely correct. |
Sorry, something went wrong.
|
To be fair, it was a clever patch and did pass the review from Tim Peters ;) (and the bug took quite a long time to get noticed so I guess lists are actually homogenous in general) |
Sorry, something went wrong.
|
Hi @embg, thinking back about this bug, I'm not sure this is the best fix. For inputs like |
Sorry, something went wrong.
|
@remilapeyre You're correct, and according to my measurements from the original PR, the performance gain would be about 40% for the case you mention. How about like this? (See changes below) |
Sorry, something went wrong.
|
@remilapeyre On second thought, I'm not clear how the early exit would impact the change proposed in my review, since the check is outside the loop. Could you walk me through that? Although I agree that two variables is a good solution if we can't patch it by just modifying a single line. |
Sorry, something went wrong.
|
Unless I missed something, you are suggesting to use this code: Then for input like If you break from the list before the third element, how can you be sure it will be a tuple? |
Sorry, something went wrong.
|
OK, I see what you mean now. Couldn't we fix it like this? That way we break early for the non-tuple case, but for tuples we check all the way through. The full diff would then be two lines. |
Sorry, something went wrong.
|
@tim-one and @rhettinger, since you were involved in the original change, could you review this please? |
Sorry, something went wrong.
|
@embg yes, this seems to be the correct condition to exit the loop. |
Sorry, something went wrong.
|
@ned-deily I haven't chimed in here because it appears to be a shallow bug that's already getting first-class attention. The kinds of bugs we were looking for at the start were far subtler 😉 . |
Sorry, something went wrong.
|
Serhiy, I would appreciate it if you could go through this as well. |
Sorry, something went wrong.
Co-Authored-By: remilapeyre <remi.lapeyre@henki.fr>
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Oh, you have changed too much. This change adds a pessimization. I suggested to fix an indentation, not to change a code.
Sorry, something went wrong.
|
Sorry, I will fix it. |
Sorry, something went wrong.
|
@remilapeyre Since |
Sorry, something went wrong.
|
Thanks for invitation but it was Lyn Levenick that reported the issue on https://bugs.python.org/issue36218#msg337341 |
Sorry, something went wrong.
|
Ah, I will post on the bugtracker thread then |
Sorry, something went wrong.
|
Let's see if this can get wrapped up in time for the alpha release this weekend. |
Sorry, something went wrong.
|
Thanks @remilapeyre for the PR, and @rhettinger for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
Sorry, something went wrong.
https://bugs.python.org/issue36218