Skip to content

Add note about module destructors #733

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

Closed
wants to merge 1 commit into from

Conversation

virtuald
Copy link
Contributor

- Technique provided by @jagerman
- Fixes pybind#638
@wjakob
Copy link
Member

wjakob commented Mar 16, 2017

This looks good. Quick question: can't the unused value simply be nullptr?

@jagerman
Copy link
Member

If I recall correctly, Python won't allow a nullptr for it. Of course, if you have some meaningful value, you should use it instead.

@dean0x7d
Copy link
Member

Would Py_None work?

@virtuald
Copy link
Contributor Author

Looking at the API docs for capsule:

  • The pointer argument may not be NULL
  • It's a void*, so it can be anything you want it to be

Unless someone tries to get the pointer, I don't believe the API will ever try to dereference it, so in theory you could do Py_None if you wanted.

@jagerman
Copy link
Member

jagerman commented Mar 18, 2017

Instead of a static local variable, how about amending the example to:

// the capsule needs a non-null pointer to reference; if you don't have anything better, create a simple one:
int *data = new int(123);

py::capsule cleanup(data, [](PyObject * capsule) {
    int *data = PyCapsule_GetPointer(capsule, nullptr);
    // do cleanup here -- this function is called with the GIL held
    delete data;
});

which also suggests an obvious way of how to pass data into the capsule.

You should also add a note that py::capsule requires either a lambda without capture (as in the example), or a regular function reference, and must not return a value (i.e. must be void).

@wjakob
Copy link
Member

wjakob commented Mar 20, 2017

I was surprised at how inelegant our way of dealing with py::capsule destructors is ;)

I submitted a separate PR to deal with this before we document the API and then can't change it anymore #752.

@wjakob
Copy link
Member

wjakob commented Mar 20, 2017

Can you update the PR to take the future changes into account?

Thanks!

wjakob added a commit to wjakob/pybind11 that referenced this pull request Mar 22, 2017
@wjakob
Copy link
Member

wjakob commented Mar 22, 2017

closing in favor of #752

@wjakob wjakob closed this Mar 22, 2017
wjakob added a commit to wjakob/pybind11 that referenced this pull request Mar 22, 2017
wjakob added a commit that referenced this pull request Mar 22, 2017
* nicer py::capsule destructor mechanism
* added destructor-only version of capsule & tests
* added documentation for module destructors (fixes #733)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants