| msg84864 - (view) |
Author: Larry Hastings (larry) *  |
Date: 2009-03-31 19:07 |
| The CObject API has two flaws. First, there is no usable type safety mechanism. You can store a void *object, and a void *description. There is no established schema for the description; it could be an integer cast to a pointer, or it could point to memory of any configuration, or it could be NULL. Thus users of the CObject API generally ignore it--thus working without any type safety whatsoever. A programmer could crash the interpreter from pure Python by mixing and matching CObjects from different modules (e.g. give "curses" a CObject from "_ctypes"). Second, the destructor callback is defined as taking *either* one *or* two parameters, depending on whether the "descr" pointer is non-NULL. One can debate the finer points of what is and isn't defined behavior in C, but at its heart this is a sloppy API. MvL and I discussed this last night and decided to float a revision of the API. I wrote the patch, though, so don't blame Martin if you don't like my specific approach. The color of this particular bike shed is: * The PyCObject is now a private data structure; you must use accessors. I added accessors for all the members. * The constructors and the main accessor (PyCObject_AsVoidPtr) now all *require* a "const char *type" parameter, which must be a non-NULL C string of non-zero length. If you call that accessor and the "type" is invalid *or doesn't match,* it fails. * The destructor now takes the PyObject *, not the PyCObject *. You must use accessors to get your hands on the data inside to free it. Yes, you can easily skip around the "matching type" restriction by calling PyCObject_AsVoidPtr(cobj, PyCObject_GetType(cobj)). The point of my API changes is to *encourage* correct use. The attached patch was written py3k/trunk r70718. It compiles with no new warnings/errors and doesn't seem to cause any new failures in the regression test. Note: this patch is not complete; I fixed all the .c and .h files, but I have yet to update the documentation. I figure I don't want to put the effort in until the dust settles. |
|
|
| msg84927 - (view) |
Author: Lisandro Dalcin (dalcinl) |
Date: 2009-03-31 21:48 |
| Two questions: 1) Why do you prefer to pass destructor as a first argument? 2) Do we really need two different calls for providing a context pointer? Why no just one call and pass a NULL context? A comment: Suppose one wants to implement export/import module C-API's in a function-by-function basis. This is nice, as you can extend your module C-API, and make it be backward "ABI" compatible. As the void* <-> void(*)(void) conversion is illegal(?) in C(99?), one has to go to the oddities of doing some sort of type-punning with unions... this could be somewhat tedious for hand-written extension modules. Do you have any idea on how extend the CObject API for the commented use case? |
|
|
| msg85074 - (view) |
Author: Larry Hastings (larry) * |
Date: 2009-04-01 18:52 |
| dalcinl: I made the destructor the first argument because APIs where arguments move around irritate me. In the existing CObject API, the destructor is "always" the last argument--which means in practice it is either the second or third argument, depending on which one you call. I also think that the destructor is a different kind of argument from the rest; the others are data attributes for the object, and the destructor is a callback. I wanted to group the data attributes together. We don't really need two different calls for providing a context pointer, and if folks thought the shorter call should go we can get rid of it. But 90% of the time all you'll need . Also, the new API grew out of the existing API, which had a similar two calls (PyCObject_FromVoidPtr, and PyCObject_FromVoidPtrAndDesc). As for your "comment", I see CObject as a lightweight way of making a generic "handle" type to represent C data structures without going to the trouble of a real PyTypeObject. I think the "storing functions in a vtable then hiding it in the Python symbol table for other modules" use case is a misuse of the API. While I'm not going to actively prevent it, I certainly wouldn't do anything to support it. |
|
|
| msg85075 - (view) |
Author: Larry Hastings (larry) * |
Date: 2009-04-01 18:59 |
| Sorry, in editing I forgot to finish my sentence. In the middle of the second paragraph, the sentence should read: But 90% of the time all you'll need is PyCObject_FromVoidPtr(). My other editing mistakes will just have to stand. |
|
|
| msg85259 - (view) |
Author: Larry Hastings (larry) * |
Date: 2009-04-02 20:02 |
| Having slept on it, I agree that we only need one API to create an object. Since passing in a "context" will be relatively rare, users who need that can use PyCObject_SetContext(). I'll remove PyCObject_FromVoidPtrWithContext() in my next patch. |
|
|
| msg85619 - (view) |
Author: Larry Hastings (larry) * |
Date: 2009-04-06 07:58 |
| I discussed this off-list with GvR. He was primarily concerned with fixing the passing-around-a-vtable C API usage of CObject, but he wanted to preserve as much backwards compatibility as possible. In the end, he suggested I create a new API and leave CObject unchanged. I've done that, incorporating many of GvR's suggestions, though the blame for the proposed new API is ultimately mine. The new object is called a "Capsule". (I *had* wanted to call it "Wrapper", but there's already a PyWrapper_New in descrobject.h.) Highlights of the new API: * PyCapsule_New() replaces PyCObject_FromVoidPtr. * It takes a void * pointer, a const char *name, and a destructor. * The pointer must not be NULL. * The name may be NULL; if it is not NULL, it must be a valid C string which outlives the capsule. * The destructor takes a PyObject *, not a void *. * PyCapsule_GetPointer() replaces PyCObject_AsVoidPtr. * It takes a PyObject * and a const char *name. * The name must compare to the name inside the object; either they're both NULL or they strcmp to be the same. * PyCapsule_Import() replaces PyCObject_Import. * It takes three arguments: const char *module_name, const char *attribute_name, int no_block. * It ensures that the "name" of the Capsule is "modulename.attributename". * If no_block is true, it uses PyModule_ImportModuleNoBlock. * The PyCapsule structure is private. There are accessors for all fields: pointer, name, destructor, and "context" * The "context" is a second "void *" you can set / get. I've attached a patch to implement this; it was written against svn r71304. The patch isn't ready to be applied--there is no documentation for the new API beyond the header file. GvR and I disagree on one point: he thinks that we should leave CObject in forever, undeprecated. I think we should deprecate it now and remove it... whenever we'd do that. The new API does everything the old one does, and more, and it's cleaner and safer. |
|
|
| msg86023 - (view) |
Author: Larry Hastings (larry) * |
Date: 2009-04-16 11:48 |
| I've updated the patch: * Capsule now has a custom repr that includes whatever name is set. * I've updated the documentation to talk about Capsules and not CObjects; the documentation discusses naming your capsules. * PyCapsule_Import now takes a "no_block" parameter; if true, PyCapsule_Import will import the module using PyImport_ImportModuleNoBlock, and if the import fails it will return failure but *not* raise an exception. * I checked all the exceptions to ensure they were of reasonable types. This is as far as I can take the patch without some input from the community. I hope the fate of this patch can be decided before the 3.1 feature freeze. |
|
|
| msg86028 - (view) |
Author: Larry Hastings (larry) * |
Date: 2009-04-16 12:56 |
| Whoopsie-daisy, I forgot to touch up a whole lot of places to match the new API. Attached is an updated patch; here's what changed: * The documentation is now much better; there is a capsule.rst, and cobject.rst has a deprecation warning. * Added a deprecation warning in a comment to Include/cobject.h. * Added the PyCapsule APIs to refcounts.dat and the .def files for OS/2 builds. * Changed a commented mention of "CObject" to "Capsule" in Lib/test/test_sys.py. |
|
|
| msg86834 - (view) |
Author: Larry Hastings (larry) * |
Date: 2009-04-29 23:13 |
| Added a test case for capsules to _testcapimodule.c, and updated to the latest trunk. |
|
|
| msg87106 - (view) |
Author: Larry Hastings (larry) * |
Date: 2009-05-04 08:39 |
| Updated patch based on impressively thorough Rietveld feedback from Benjamin Peterson. Thanks, Benjamin! This patch should not be considered final; we already know the API documentation in the comments in Include/pycapsule.h needs to change (somehow). |
|
|
| msg87135 - (view) |
Author: Lisandro Dalcin (dalcinl) |
Date: 2009-05-04 15:48 |
| In Doc/c-api/capsule.rst, you wrote .. cfunction:: int PyCapsule_Import(const char* name, int no_block) but it should be: .. cfunction:: void* PyCapsule_Import(const char* name, int no_block) Additionally, you wrote "disambugate" in many places. |
|
|
| msg87213 - (view) |
Author: Larry Hastings (larry) * |
Date: 2009-05-05 07:14 |
| dalcinl: Thanks, I've fixed the doc wrt writing "int" where I meant "void *". As for using "disambiguate" in many places: there are a couple of sentences that are repeated in the documentation for several functions. Those repeated sentences all use "ambiguous" and "disambiguate". Having them read almost exactly the same is a subtle cue to the reader, suggesting they will behave similarly, which is correct. Is that a problem? |
|
|
| msg87220 - (view) |
Author: Larry Hastings (larry) * |
Date: 2009-05-05 08:06 |
| A nice fresh patch, against r72309. Incorporates changes based on Benjamin's latest batch of Rietveld comments. They're thinning out, so we must be near the end--and with a day to spare. Also strips out almost-all documentation from "pycapsule.h". Note: this still has CObject flagged as deprecated, both in doc and with a runtime warning. Dare I hope these will survive into the accepted patch? |
|
|
| msg87296 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2009-05-05 22:32 |
| Applied in r72363. |
|
|