Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create alternative CObject API that is safe and clean #49880

Closed
larryhastings opened this issue Mar 31, 2009 · 14 comments
Closed

Create alternative CObject API that is safe and clean #49880

larryhastings opened this issue Mar 31, 2009 · 14 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@larryhastings
Copy link
Contributor

BPO 5630
Nosy @jcea, @larryhastings, @devdanzin, @benjaminp
Files
  • cobject.diff: Patch against py3k/trunk r70718.
  • lch.capsule.r71304.diff: Patch against py3k/trunk r71304.
  • lch.capsule.r71641.diff: Patch against py3k/trunk svn r71641.
  • lch.capsule.r71641.diff.2: Patch against py3k/trunk svn r71641, revision 2.
  • lch.capsule.r72125.diff: Patch against py3k/trunk r72125.
  • lch.capsule.r72270.diff: Patch against py3k/trunk r72270.
  • lch.capsule.r72309.diff: Patch against py3k/trunk r72309.
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2009-05-05.22:32:26.799>
    created_at = <Date 2009-03-31.19:07:48.893>
    labels = ['interpreter-core', 'type-feature']
    title = 'Create alternative CObject API that is safe and clean'
    updated_at = <Date 2010-04-25.03:15:29.386>
    user = 'https://github.com/larryhastings'

    bugs.python.org fields:

    activity = <Date 2010-04-25.03:15:29.386>
    actor = 'jcea'
    assignee = 'none'
    closed = True
    closed_date = <Date 2009-05-05.22:32:26.799>
    closer = 'benjamin.peterson'
    components = ['Interpreter Core']
    creation = <Date 2009-03-31.19:07:48.893>
    creator = 'larry'
    dependencies = []
    files = ['13521', '13626', '13703', '13706', '13820', '13864', '13892']
    hgrepos = []
    issue_num = 5630
    keywords = ['patch']
    message_count = 14.0
    messages = ['84864', '84927', '85074', '85075', '85259', '85619', '86023', '86028', '86834', '87106', '87135', '87213', '87220', '87296']
    nosy_count = 5.0
    nosy_names = ['jcea', 'dalcinl', 'larry', 'ajaksu2', 'benjamin.peterson']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = None
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue5630'
    versions = ['Python 3.1']

    @larryhastings
    Copy link
    Contributor Author

    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.

    @larryhastings larryhastings added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Mar 31, 2009
    @dalcinl
    Copy link
    Mannequin

    dalcinl mannequin commented Mar 31, 2009

    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?

    @devdanzin devdanzin mannequin added the type-feature A feature request or enhancement label Apr 1, 2009
    @larryhastings
    Copy link
    Contributor Author

    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.

    @larryhastings
    Copy link
    Contributor Author

    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.

    @larryhastings
    Copy link
    Contributor Author

    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.

    @larryhastings
    Copy link
    Contributor Author

    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.

    @larryhastings larryhastings changed the title Update CObject API so it is safe and regular Create alternatieve CObject API that is safe and clean Apr 6, 2009
    @larryhastings
    Copy link
    Contributor Author

    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.

    @larryhastings
    Copy link
    Contributor Author

    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.

    @larryhastings
    Copy link
    Contributor Author

    Added a test case for capsules to _testcapimodule.c, and updated to the
    latest trunk.

    @larryhastings larryhastings changed the title Create alternatieve CObject API that is safe and clean Create alternative CObject API that is safe and clean Apr 29, 2009
    @larryhastings
    Copy link
    Contributor Author

    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).

    @dalcinl
    Copy link
    Mannequin

    dalcinl mannequin commented May 4, 2009

    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.

    @larryhastings
    Copy link
    Contributor Author

    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?

    @larryhastings
    Copy link
    Contributor Author

    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?

    @benjaminp
    Copy link
    Contributor

    Applied in r72363.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants