bpo-30341: Add an explaining comment in _PyTrash_thread_destroy_chain() #1545
Conversation
|
@zhangyangyu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @benjaminp, @tim-one and @serhiy-storchaka to be potential reviewers. |
Objects/object.c
Outdated
| * up distorting allocation statistics. | ||
| */ | ||
| assert(op->ob_refcnt == 0); | ||
| /* The deallocator executing this function still occupies |
There was a problem hiding this comment.
I am confused about the first sentence, "The deallocator executing this function still occupies the stack". Otherwise, LGTM.
There was a problem hiding this comment.
What I want to say is the "root deallocator" is still doing job and the stack is not released yet. So counting it into trash_delete_nesting is natural. And I also want to list the example you give here. Is it okay?
There was a problem hiding this comment.
So counting it into trash_delete_nesting is natural.
That's not really what matters here, though. What the increment avoids is the possibility of an arbitrary deep recursion in _PyTrash_thread_destroy_chain.
And I also want to list the example you give here. Is it okay?
Yes!
There was a problem hiding this comment.
Delete the first sentence and add the example. :-)
Objects/object.c
Outdated
| del tups | ||
| */ | ||
| assert(tstate->trash_delete_nesting == 0); | ||
| ++tstate->trash_delete_nesting; |
There was a problem hiding this comment.
Why not move the increasing/decreasing outside of the loop?
There was a problem hiding this comment.
Sorry, misunderstood your comment and gave a wrong reply. Hmm, I cannot think of a reason why not. What's your opinion @pitrou ?
There was a problem hiding this comment.
It may be safe to move it out of the loop indeed. But I'm not sure it's a good idea to touch this code too much, either.
There was a problem hiding this comment.
Okay. Although touching it too much seems not wise, but Serhiy's suggestion sounds good and this will only go into 3.7, we have time to check it.
|
Thanks @pitrou and @serhiy-storchaka ! |
5 participants
No description provided.