Conversation
Tpt
left a comment
Tpt
left a comment
There was a problem hiding this comment.
I am not an expert on Capsules, so an other approval might be nice
| /// Constructs a new capsule whose contents are `value`, associated with `name`. | ||
| /// | ||
| /// Also provides a destructor: when the `PyCapsule` is destroyed, it will be passed the original object, | ||
| /// as well as a `*mut c_void` which will point to the capsule's context, if any. |
There was a problem hiding this comment.
sorry for not spotting it: this method does not allow to set any initial context to the capsule, is it worth it to add this parameter to this method or should we drop it from this method and add a new new_with_value_and_context method that would take for input an initial context and allow the destructor to destruct the context?
There was a problem hiding this comment.
I don't think we could do much more than construct the capsule and call set_context afterwards. As this can already be done, I don't think we need to add another argument here.
There was a problem hiding this comment.
Make sense! But we want to make all other users of new_with_value to care about the possibility of having a context even if it's most often not the case (I am just nitpicking here, feel free to just ignore my reply)
There was a problem hiding this comment.
I can see the point (always happy to discuss 😄). This is only for new_with_value_and_destructor however and I think we need to give the context in the destructor, so that users have a way to free it if they used it. (If you need a destructor you'd probably have context anyway and if not it's easy to ignore)
There was a problem hiding this comment.
I think I'd prefer have context: Option<*mut c_void> as an argument to these functions rather than new flavours of the method, however I also agree that probably just calling set_context after is probably good enough?
Also context is somewhat less useful here than for C callers, because we are already storing additional context in the destructor closure.
There was a problem hiding this comment.
I'd propose we leave it as is then.
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
Thanks for moving this forward!
| /// Constructs a new capsule whose contents are `value`, associated with `name`. | ||
| /// | ||
| /// Also provides a destructor: when the `PyCapsule` is destroyed, it will be passed the original object, | ||
| /// as well as a `*mut c_void` which will point to the capsule's context, if any. |
There was a problem hiding this comment.
I think I'd prefer have context: Option<*mut c_void> as an argument to these functions rather than new flavours of the method, however I also agree that probably just calling set_context after is probably good enough?
Also context is somewhat less useful here than for C callers, because we are already storing additional context in the destructor closure.
3 participants
Closes #5692