NotificationsYou must be signed in to change notification settings
Fork33.7k
Star70.4k

bpo-40077: Convert _csv module to use PyType_FromSpec#20974

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

corona10 wants to merge7 commits intopython:masterfromcorona10:bpo-40077-csv

Closed

bpo-40077: Convert _csv module to use PyType_FromSpec#20974

corona10 wants to merge7 commits intopython:masterfromcorona10:bpo-40077-csv

Conversation

Copy link

Member

corona10 commentedJun 19, 2020•
edited by bedevere-bot
Loading

https://bugs.python.org/issue40077

corona10 added2 commits

June 19, 2020 12:09

bpo-40077: Convert _csv module to use PyType_FromSpec

f80d4c6

bpo-40077: Add NEWS.d

53df5eb

the-knights-who-say-ni added the CLA signed label

Jun 19, 2020

bedevere-bot added the awaiting core review label

Jun 19, 2020

corona10 added the DO-NOT-MERGE label

Jun 19, 2020

corona10 added2 commits

June 19, 2020 15:02

bpo-40077: Update

dbc4d32

bpo-40077: PEP7

9f1266b

corona10 removed the DO-NOT-MERGE label

Jun 19, 2020

corona10 requested a review fromvstinner

June 19, 2020 06:14

corona10 commented

Jun 19, 2020

View reviewed changes

Copy link

MemberAuthor

corona10 left a comment

Choose a reason for hiding this comment

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

@vstinner
Please take a look

I locally tested with _csv module on subinterpreter and module test
And there was no leak.
(But import csv with subinterpreter still has leaks but not related to this PR)

Copy link

MemberAuthor

corona10 commentedJun 19, 2020

FYI, macOS CI issue is not related to this PR

vstinner reviewed

Jun 19, 2020

View reviewed changes

Modules/_csv.c OutdatedShow resolvedHide resolved

Modules/_csv.cShow resolvedHide resolved

Modules/_csv.c OutdatedShow resolvedHide resolved

corona10 added3 commits

June 19, 2020 19:26

bpo-40077: Update

85d36e9

bpo-40077: Update

10114f0

bpo-40077: Apply Victor's suggestion

14c6de7

corona10 requested a review fromvstinner

June 19, 2020 10:39

corona10 commented

Jun 19, 2020

View reviewed changes

Copy link

MemberAuthor

corona10 left a comment

Choose a reason for hiding this comment

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

@vstinner

Thanks all of your reviews are applied and no memory leak was found!

vstinner requested changes

Jun 19, 2020

View reviewed changes

Copy link

Member

vstinner left a comment

Choose a reason for hiding this comment

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

I'm not sure that it's possible to convert static types to heap types if they have Py_TPFLAGS_BASETYPE, since currently there is no way to retrieve the module from such type.

Modules/_csv.cShow resolvedHide resolved

Modules/_csv.c

		&strict))
		returnNULL;

		_csvstate*state=PyType_GetModuleState(type);

Copy link

Member

vstinnerJun 19, 2020

Choose a reason for hiding this comment

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

PyType_GetModuleState() is not safe if the type has Py_TPFLAGS_BASETYPE flag, which is the case here.

Is there a way to get the defining type in tp_new?

Modules/_csv.c

		PyErr_Format(_csvstate_global->error_obj,"field larger than field limit (%ld)",
		_csvstate_global->field_limit);
		PyTypeObject*reader_type=Py_TYPE(self);
		_csvstate*state=PyType_GetModuleState(reader_type);