| msg39496 - (view) |
Author: Neil Schemenauer (nascheme) *  |
Date: 2002-04-07 01:07 |
| This patch removes all PyMalloc_* symbols from the source. obmalloc now implements PyObject_{Malloc, Realloc, Free}. PyObject_{New,NewVar} allocate using pymalloc. I also changed PyObject_Del and PyObject_GC_Del so that they be used as function designators. Is changing the signature of PyObject_Del going to cause any problems? I had to add some extra typecasts when assigning to tp_free. Please review and assign back to me. The next phase would be to cleanup the memory API usage. Do we want to replace all PyObject_Del calls with PyObject_Free? PyObject_Del seems to match better with PyObject_GC_Del. Oh yes, we also need to change PyMem_{Free, Del, ...} to use pymalloc's free. |
|
|
| msg39497 - (view) |
Author: Tim Peters (tim.peters) * |
Date: 2002-04-07 02:40 |
| Logged In: YES user_id=31435 Looks good to me -- thanks! |
|
|
| msg39498 - (view) |
Author: Tim Peters (tim.peters) * |
Date: 2002-04-07 02:41 |
| Logged In: YES user_id=31435 Oops -- I hit "Submit" prematurely. More to come. |
|
|
| msg39499 - (view) |
Author: Tim Peters (tim.peters) * |
Date: 2002-04-07 02:59 |
| Logged In: YES user_id=31435 Extensions that *currently* call PyObject_Del have its old macro expansion ("_PyObject_Del((PyObject *)(op))") buried in them, so getting rid of _PyObject_Del is a binary-API incompatibility (existing extensions will no longer link without recompilation). I personally don't mind that, but I run on Windows and "binary compatability" never works there across minor releases for other reasons, so I don't have any real feel for how much people on other platforms value it. As you pointed out recently too, binary compatability has, in reality, not been the case since 1.5.2 anyway. So that's one for Python-Dev. If we do break binary compatibility, I'd be sorely tempted to change the "destructor" typedef to say destructors take void*. IMO saying they take PyObject* was a poor idea, as you almost never have a PyObject* when calling one of these guys. That's why PyObject_Del "had to" be a macro, to hide the cast to PyObject* almost everyone needs because of destructor's "correct" but impractical signature. If "destructor" had a practical signature, there would have been no temptation to use a macro. Note that if the typedef of destructor were so changed, you wouldn't have needed new casts in tp_free slots. And I'd rather break binary compatability than make extension authors add new casts. Hmm. I'm assigning this to Guido for comment: Guido, what are your feelings about binary compatibility here? C didn't define free() as taking a void* by mistake . Back to Neil: I wouldn't bother changing PyObject_Del to PyObject_Free. The former isn't in the "recommended" minimal API, but neither is it discouraged. I expect TMTOWTDI here forever. |
|
|
| msg39500 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2002-04-08 18:47 |
| Logged In: YES user_id=6380 I'm looking at this now... |
|
|
| msg39501 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2002-04-08 19:18 |
| Logged In: YES user_id=6380 (Wouldn't it be more efficient to take this to email between the three of us?) > Extensions that *currently* call PyObject_Del have > its old macro expansion ("_PyObject_Del((PyObject > *)(op))") buried in them, so getting rid of > _PyObject_Del is a binary-API incompatibility > (existing extensions will no longer link without > recompilation). I personally don't mind that, but > I run on Windows and "binary compatability" never > works there across minor releases for other > reasons, so I don't have any real feel for how > much people on other platforms value it. As you > pointed out recently too, binary compatability > has, in reality, not been the case since 1.5.2 > anyway. Still, tradition has it that we keep such entry points around for a long time. I propose that we do so now, too. > So that's one for Python-Dev. If we do break > binary compatibility, I'd be sorely tempted to > change the "destructor" typedef to say destructors > take void*. IMO saying they take PyObject* was a > poor idea, as you almost never have a PyObject* > when calling one of these guys. Huh? "destructor" is used to declare tp_dealloc, which definitely needs a PyObject * (or some "subclass" of it, like PyIntObject *). It's also used to declare tp_free, which arguably shouldn't take a PyObject * (since by the time tp_free is called, most of the object's contents have been destroyed by tp_dealloc). So maybe tp_free (a newcomer in 2.2) should be declared to take something else, but then the risk is breaking code that defines a tp_free with the correct signature. > That's why PyObject_Del "had to" be a macro, to > hide the cast to PyObject* almost everyone needs > because of destructor's "correct" but impractical > signature. If "destructor" had a practical > signature, there would have been no temptation to > use a macro. I don't understand this at all. > Note that if the typedef of destructor were so > changed, you wouldn't have needed new casts in > tp_free slots. And I'd rather break binary > compatability than make extension authors add new > casts. Nor this. > Hmm. I'm assigning this to Guido for comment: > Guido, what are your feelings about binary > compatibility here? C didn't define free() as > taking a void* by mistake . I want binary compatibility, but I don't understand your comments very well. > Back to Neil: I wouldn't bother changing PyObject_Del > to PyObject_Free. The former isn't in the > "recommended" minimal API, but neither is it > discouraged. I expect TMTOWTDI here forever. I prefer PyObject_Del -- like PyObject_GC_Del, and like we did in the past. Plus, I like New to match Del and Malloc to match Free. Since it's PyObject_New, it should be _Del. I'm not sure what to say of Neil's patch, except that I'm glad to be rid of the PyMalloc_XXX family. I wish we didn't have to change all the places that used to say _PyObject_Del. Maybe it's best to keep that name around? The patch would (psychologically) become a lot smaller. I almost wish that this would work: #define PyObject_Del ((destructor)PyObject_Free) Or maybe it *does* work??? |
|
|
| msg39502 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2002-04-09 16:27 |
| Logged In: YES user_id=6380 I've not fully read Tim's response in email, but instead I've reviewed and discussed the patch with Tim. I think the only thing to which I object at this point is the removal of the entry point _PyObject_Del. I believe that for source and binary compatibility with 2.2, that entry point should remain, with the same meaning, but it should not be used at all by the core. (Motivation to keep it: it's the only thing you can reasonably stick in tp_free that works for 2.2 as well as for 2.3.) One minor question: there are a bunch of #undefs in gcmodule.c (e.g. PyObject_GC_Track) that don't seem to make sense -- at least I cannot find where these would be #defined any more. Ditto for #indef PyObject_Malloc in obmalloc.c. I suggest that you check this thing in, but keeping _PyObject_Del alive, and we'll take it from there. |
|
|
| msg39503 - (view) |
Author: Tim Peters (tim.peters) * |
Date: 2002-04-09 18:47 |
| Logged In: YES user_id=31435 Clarifying or just repeating Guido here: + Binary compatibility is important. It's better on Unix than it appears -- while you'll get a warning if you run an old 1.5.2 extension with 2.2 today and without recompiling, it will almost certainly work anyway. So in the case of macros that expanded to a private API function before, that private API function must still exist, but the macro needn't expand to that anymore (nor even *be* a macro anymore). _PyObject_Del is a particular problem cuz it's even documented in the C API manual -- there simply wasn't a public API function before that did the same thing and could be used as a function designator. You're making life better for future generations. + Casts on tp_free slots are par for the course, because "destructor" has an impractical signature. I'm afraid that can't change either, so the casts stay. + Fred and I agreed to add PyObject_Del to the "minimal recommended API", so, for the next round of this, feel wholly righteous in leaving existing PyObject_Del calls alone. If anything's unclear, hit me. |
|
|
| msg39504 - (view) |
Author: Neil Schemenauer (nascheme) * |
Date: 2002-04-09 20:29 |
| Logged In: YES user_id=35752 It might be a day or two before I get to this. Regarding the type of tp_free, could we change it to be something like: typedef void (*freefunc)(void *); ... freefunc tp_free; and leave the type of tp_dealloc alone. Maybe it's too late now that 2.2 is out and uses 'destructor'. I don't see how this relates to binary compatibility though. Why does it matter if the function takes a PyObject pointer or a void pointer? The worse I see happening is that people could get warnings when they compile their extension modules. |
|
|
| msg39505 - (view) |
Author: Tim Peters (tim.peters) * |
Date: 2002-04-09 20:43 |
| Logged In: YES user_id=31435 It'll be a day or two before PLabs can get back to Python work anyway. Reassigning to Guido -- I'm not even going to try to channel him on backwards compatibility, or the feasibility of introducing possible warnings. If I were you I'd check in the patch with the casts in; they can be taken out again later if Guido is agreeable. |
|
|
| msg39506 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2002-04-10 00:53 |
| Logged In: YES user_id=6380 The binary compatibility issue is extensions compiled for 2.2 that have references to _PyObject_Del compiled into them and aren't recompiled for 2.3. I think that should work (even if they get a warning). To make it work, the _PyObject_Del entry point must continue to exist. Back to Neil, I think my instructions are clear enough. |
|
|
| msg39507 - (view) |
Author: Neil Schemenauer (nascheme) * |
Date: 2002-04-18 02:19 |
| Logged In: YES user_id=35752 A modified version of the patch has been commited. |
|
|