classification
Title: Create alternative CObject API that is safe and clean
Type: enhancement Stage:
Components: Interpreter Core Versions: Python 3.1
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: Nosy List: ajaksu2, benjamin.peterson, dalcinl, jcea, larry
Priority: normal Keywords: patch

Created on 2009-03-31 19:07 by larry, last changed 2010-04-25 03:15 by jcea. This issue is now closed.

Files
File name Uploaded Description Edit
cobject.diff larry, 2009-03-31 19:07 Patch against py3k/trunk r70718.
lch.capsule.r71304.diff larry, 2009-04-06 07:58 Patch against py3k/trunk r71304.
lch.capsule.r71641.diff larry, 2009-04-16 11:48 Patch against py3k/trunk svn r71641.
lch.capsule.r71641.diff.2 larry, 2009-04-16 12:56 Patch against py3k/trunk svn r71641, revision 2.
lch.capsule.r72125.diff larry, 2009-04-29 23:13 Patch against py3k/trunk r72125.
lch.capsule.r72270.diff larry, 2009-05-04 08:39 Patch against py3k/trunk r72270.
lch.capsule.r72309.diff larry, 2009-05-05 08:06 Patch against py3k/trunk r72309.
Messages (14)
msg84864 - (view) Author: Larry Hastings (larry) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2009-05-05 22:32
Applied in r72363.
History
Date User Action Args
2010-04-25 03:15:29jceasetnosy: + jcea
2009-05-05 22:32:26benjamin.petersonsetstatus: open -> closed

nosy: + benjamin.peterson
messages: + msg87296

resolution: accepted
2009-05-05 08:06:48larrysetfiles: + lch.capsule.r72309.diff

messages: + msg87220
2009-05-05 07:14:27larrysetmessages: + msg87213
2009-05-04 15:48:24dalcinlsetmessages: + msg87135
2009-05-04 08:40:04larrysetfiles: + lch.capsule.r72270.diff

messages: + msg87106
2009-04-29 23:13:38larrysetfiles: + lch.capsule.r72125.diff

messages: + msg86834
title: Create alternatieve CObject API that is safe and clean -> Create alternative CObject API that is safe and clean
2009-04-16 12:56:39larrysetfiles: + lch.capsule.r71641.diff.2

messages: + msg86028
2009-04-16 11:48:38larrysetfiles: + lch.capsule.r71641.diff

messages: + msg86023
2009-04-06 07:58:43larrysetfiles: + lch.capsule.r71304.diff

messages: + msg85619
title: Update CObject API so it is safe and regular -> Create alternatieve CObject API that is safe and clean
2009-04-02 20:02:41larrysetmessages: + msg85259
2009-04-01 18:59:54larrysetmessages: + msg85075
2009-04-01 18:52:36larrysetmessages: + msg85074
2009-04-01 03:07:22ajaksu2setnosy: + ajaksu2
type: enhancement
2009-03-31 21:49:00dalcinlsetnosy: + dalcinl
messages: + msg84927
2009-03-31 19:07:49larrycreate