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

fix: add missing signature #2363

Merged
merged 2 commits into from
Aug 14, 2020
Merged

fix: add missing signature #2363

merged 2 commits into from
Aug 14, 2020

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Aug 4, 2020

All the other array constructors have an optional handle, but this one was missing.

@YannickJadoul
Copy link
Collaborator

I was about to say "Isn't that because it's a constructor from buffer_info?", but apparently buffer_info doesn't contain a base either? I'm not a buffer-protocol expert, though, so I don't know if it should be in there. If not, this seems fine.

Maybe do the same for the array_t constructor, though?

@henryiii
Copy link
Collaborator Author

henryiii commented Aug 4, 2020

@henryiii
Copy link
Collaborator Author

henryiii commented Aug 4, 2020

Someone told me when I found this a while back it was probably just an oversight, but I don't know where that conversation was.

@henryiii
Copy link
Collaborator Author

henryiii commented Aug 4, 2020

By the way, buffer info is information about the data, but it does not contain ownership information. The handle contains ownership information, so you can be sure the owning object is not destroyed while the array is still present. The other constructors support this, it's just missing from the direct buffer info constructor.

@YannickJadoul
Copy link
Collaborator

Alright; Py_buffer does contain a object field, but py::buffer_info doesn't. I don't know enough about the buffer protocol to assess the details of what should do what, but adding these optional base arguments to the signature seems fine to me!

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!
I spent a few minutes looking in the nump.h sources, this change seems super safe to me.

@henryiii henryiii merged commit 2e2de8c into pybind:master Aug 14, 2020
@henryiii henryiii deleted the feat/missingsig branch August 14, 2020 00:13
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.

3 participants