Skip to content

bpo-40077: Convert _jsonmodule to use PyType_FromSpec. - #19177

Merged
vstinner merged 6 commits into
python:masterfrom
corona10:bpo-40077-json
Mar 27, 2020
Merged

bpo-40077: Convert _jsonmodule to use PyType_FromSpec.#19177
vstinner merged 6 commits into
python:masterfrom
corona10:bpo-40077-json

Conversation

@corona10

@corona10corona10 commented Mar 26, 2020•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@corona10
corona10 requested a review from vstinnerMarch 26, 2020 16:26
vstinner
vstinner reviewed Mar 26, 2020
View reviewed changes
Comment threadModules/_json.c Outdated
Comment threadModules/_json.c Outdated
Comment threadModules/_json.c
Comment threadModules/_json.c Outdated
Comment threadModules/_json.c Outdated
Comment threadModules/_json.c Outdated
Comment threadModules/_json.c Outdated
Comment threadModules/_json.c Outdated
@corona10
corona10 requested a review from vstinnerMarch 26, 2020 19:23
vstinner
vstinner reviewed Mar 26, 2020
View reviewed changes
Comment threadModules/_json.c
encoder_clear(self);
Py_TYPE(self)->tp_free(self);
encoder_clear((PyEncoderObject *)self);
tp->tp_free(self);

@vstinnervstinnerMar 26, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code is just fine, no? I don't see the value of tp. It seems like it comes from a previous change that you reverted.

Suggested change
tp->tp_free(self);
Py_TYPE(self)->tp_free(self);

@corona10corona10Mar 27, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vstinner
Py_DECREF(tp);is needed if not test is leaked.
This is why I declared tp ;)

@corona10corona10Mar 27, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

test_json leaked [114, 114, 114] references, sum=342
test_json failed in 35.6 sec

Comment threadModules/_json.c Outdated
Comment threadModules/_json.c Outdated
Comment threadModules/_json.c
Comment threadModules/_json.c Outdated
@corona10
corona10 requested a review from vstinnerMarch 27, 2020 03:16
@corona10

corona10 commented Mar 27, 2020

Copy link
Copy Markdown
MemberAuthor

@vstinner
Thanks for the review. :)
I 've applied all of your comment except #19177 (comment) :)

Please take a look

@vstinner
vstinner merged commit 33f15a1 into python:masterMar 27, 2020
@vstinner

vstinner commented Mar 27, 2020

Copy link
Copy Markdown
Member

Thanks @corona10, this change was interesting. I learnt a few things :-) I merged your PR, but I completed the commit message to elaborate on changes that you wrote.

@vstinner

vstinner commented Apr 1, 2020•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

I mentioned the removed assertions in bpo-40137 that I just created: TODO list when PEP 573 "Module State Access from C Extension Methods" will be implemented.

@corona10corona10 mentioned this pull request Jun 27, 2022
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinnervstinnerAwaiting requested review from vstinner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@corona10@vstinner@the-knights-who-say-ni@bedevere-bot