classification
Title: enhance getargs O& to accept cleanup function
Type: resource usage Stage: patch review
Components: Interpreter Core Versions: Python 3.1, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, loewis, ocean-city, pitrou
Priority: release blocker Keywords: patch

Created on 2009-05-13 11:46 by ocean-city, last changed 2009-05-29 14:50 by loewis. This issue is now closed.

Files
File name Uploaded Description Edit
experimental_getargs_with_cleanup.patch ocean-city, 2009-05-13 11:46
experimental_getargs_with_cleanup_v2.patch ocean-city, 2009-05-14 12:29
conv.diff loewis, 2009-05-17 07:06
Messages (14)
msg87677 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-05-13 11:46
This issue comes from #5990.

Currently, O& doesn't accept general cleanup function. This causes
memory leak sometimes. Here is an experimental patch to create new
format O&& similar to O& but have general cleanup function.
msg87700 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-05-13 18:44
I would propose a different approach: the same function can be both
argument parser and cleanup function. In parsing, the PyObject* points
to the parameter being converted. In cleanup, it points to NULL.

This would allow the O& to continue to be used even if cleanup is
needed. As not all converters would need or support cleanup, converters
that do need cleanup could be required to return Py_CLEANUP_SUPPORTED
(which would be, say, 131072).
msg87736 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-05-14 12:29
> the same function can be both
> argument parser and cleanup function

Here is an another experimental patch to implement this. (Is this right
usage of PyCObject_FromVoidPtrAndDesc?)

> As not all converters would need or support cleanup, converters
> that do need cleanup could be required to return Py_CLEANUP_SUPPORTED

Well, this is not implemented yet. (I introduced another format
temporary) Does this mean converter should return Py_CLEANUP_SUPPORTED
instead of 1 when cleanup is needed?
msg87774 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-05-14 22:04
> Well, this is not implemented yet. (I introduced another format
> temporary) Does this mean converter should return Py_CLEANUP_SUPPORTED
> instead of 1 when cleanup is needed?

If you are introducing a new character ($), then this isn't actually
needed. I was thinking of reusing O&, literally.
msg87962 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-05-17 07:06
Modifying convert_to_unicode is incorrect; this function is not an O&
converter. Instead, PyUnicode_FSConverter needs to change.

Attached is a patch that does that, and also uses the O& approach. It
also adjusts the patch to use capsules.
msg87967 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-05-17 08:41
> Modifying convert_to_unicode is incorrect; this function is not an O&
> converter. Instead, PyUnicode_FSConverter needs to change.

Well, convert_to_unicode used to use O& before, and I fixed memory leak
with O formatter in current way. I used this function because I know
about it more than PyUnicode_FSConverter. Sorry for confusion. ;-)

> Attached is a patch that does that, and also uses the O& approach. It
> also adjusts the patch to use capsules.

Oh, this is how to use new capsule functions.... I noticed capsule.c was
added, but I didn't know how to use it.
msg87985 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-05-17 10:55
> Well, convert_to_unicode used to use O& before

I don't understand. Where does it use it? Before
your patch, convert_to_unicode was a regular one-argument
function, not a ParseTuple O& function. Only your patch
makes it one.
msg87988 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-05-17 11:07
Well, please see r65745. That was O& before.
msg88285 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-05-24 21:04
So: any opinions what approach would be better? A new converter O$ or a
change to the existing O&, selected by return value from the conversion
function?
msg88286 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-24 21:18
Reusing O& looks better to me (we already have too many type specifiers). 
Why have you chosen 0x20000 for Py_CLEANUP_SUPPORTED? Are there other
possible values below that?
msg88289 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-05-24 21:39
> Reusing O& looks better to me (we already have too many type specifiers). 
> Why have you chosen 0x20000 for Py_CLEANUP_SUPPORTED? Are there other
> possible values below that?

It's essentially random. I want it to be a power of two, in case we make
it a bit mask some day, and significantly different from 1. 0x20000 was
the smallest number that qualified :-)
msg88298 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-05-25 01:23
> Reusing O& looks better to me

Me too.
msg88479 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2009-05-28 21:42
Patch review:

- It needs docs and tests.
- In addcleanup_convert, Py_FatalError calls abort, so there isn't
really a point of returning -1;
- Also in the function, you need to call destr() in the case that
PyList_Append fails.
msg88509 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-05-29 14:50
I have now committed this patch with the proposed modifications as r73014.

I don't believe that the additional destr call is necessary, as
releasing the capsule will invoke destr already.
History
Date User Action Args
2009-05-29 14:50:25loewissetstatus: open -> closed
resolution: fixed
messages: + msg88509
2009-05-28 21:42:24benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg88479
2009-05-25 01:23:05ocean-citysetmessages: + msg88298
2009-05-24 21:39:41loewissetmessages: + msg88289
2009-05-24 21:18:39pitrousetnosy: + pitrou
messages: + msg88286
2009-05-24 21:04:19loewissetmessages: + msg88285
2009-05-17 11:07:17ocean-citysetmessages: + msg87988
2009-05-17 10:55:39loewissetmessages: + msg87985
2009-05-17 08:41:21ocean-citysetmessages: + msg87967
2009-05-17 07:06:54loewissetfiles: + conv.diff

messages: + msg87962
2009-05-17 05:00:43loewissetpriority: normal -> release blocker
2009-05-14 22:04:59loewissetmessages: + msg87774
2009-05-14 12:29:13ocean-citysetfiles: + experimental_getargs_with_cleanup_v2.patch

messages: + msg87736
2009-05-13 18:44:49loewissetmessages: + msg87700
2009-05-13 15:10:09pitrousetpriority: normal
nosy: + loewis

type: resource usage
stage: patch review
2009-05-13 15:09:47pitroulinkissue5990 dependencies
2009-05-13 11:46:18ocean-citycreate