bpo-40077: Convert _csv module to use PyType_FromSpec by corona10 · Pull Request #20974 · python/cpython

corona10

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)

FYI, macOS CI issue is not related to this PR

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!

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.

		&strict))
		return NULL;

		_csvstate *state = PyType_GetModuleState(type);

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?

		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);

Choose a reason for hiding this comment

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

The Reader type has Py_TPFLAGS_BASETYPE: PyType_GetModuleState() is unsafe here.

		return NULL;
		}
		self->dialect = (DialectObj *)_call_dialect(dialect, keyword_args);
		_csvstate *state = get_csv_state(module);

Choose a reason for hiding this comment

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

state can be moved at the beginning of the function, to avoid calling get_csv_state() twice.

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

The issue is now closed via #23224.
Thank you for working on it, though!