msg87677 - (view) |
Author: Hirokazu Yamamoto (ocean-city) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2009-05-17 11:07 |
Well, please see r65745. That was O& before.
|
msg88285 - (view) |
Author: Martin v. Löwis (loewis) * |
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) * |
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) * |
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) * |
Date: 2009-05-25 01:23 |
> Reusing O& looks better to me
Me too.
|
msg88479 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
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) * |
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.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:56:48 | admin | set | github: 50262 |
2009-05-29 14:50:25 | loewis | set | status: open -> closed resolution: fixed messages:
+ msg88509
|
2009-05-28 21:42:24 | benjamin.peterson | set | nosy:
+ benjamin.peterson messages:
+ msg88479
|
2009-05-25 01:23:05 | ocean-city | set | messages:
+ msg88298 |
2009-05-24 21:39:41 | loewis | set | messages:
+ msg88289 |
2009-05-24 21:18:39 | pitrou | set | nosy:
+ pitrou messages:
+ msg88286
|
2009-05-24 21:04:19 | loewis | set | messages:
+ msg88285 |
2009-05-17 11:07:17 | ocean-city | set | messages:
+ msg87988 |
2009-05-17 10:55:39 | loewis | set | messages:
+ msg87985 |
2009-05-17 08:41:21 | ocean-city | set | messages:
+ msg87967 |
2009-05-17 07:06:54 | loewis | set | files:
+ conv.diff
messages:
+ msg87962 |
2009-05-17 05:00:43 | loewis | set | priority: normal -> release blocker |
2009-05-14 22:04:59 | loewis | set | messages:
+ msg87774 |
2009-05-14 12:29:13 | ocean-city | set | files:
+ experimental_getargs_with_cleanup_v2.patch
messages:
+ msg87736 |
2009-05-13 18:44:49 | loewis | set | messages:
+ msg87700 |
2009-05-13 15:10:09 | pitrou | set | priority: normal nosy:
+ loewis
type: resource usage stage: patch review |
2009-05-13 15:09:47 | pitrou | link | issue5990 dependencies |
2009-05-13 11:46:18 | ocean-city | create | |