-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Use std::unique_ptr in pybind11_getbuffer #5435
Conversation
@@ -605,7 +604,6 @@ extern "C" inline int pybind11_getbuffer(PyObject *obj, Py_buffer *view, int fla | |||
// Fill in all the information, and then downgrade as requested by the caller, or raise an | |||
// error if that's not possible. | |||
view->obj = obj; | |||
view->internal = info; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, but feels a little inconsistent, why move setting view->internal
to the end while setting view->buf
here from the same info
object? I would keep them both together, and it would be good to explain why it is moved to the end if that is preferred.
I think in the case of error info
is deleted and also info->ptr
.
@cryos asked about the CI / 🐍 3 • Clang 11 • C++20 • x64 timeout offline: That's a flake / infrastructure issue that popped up a couple days ago. I've been triggering reruns for several PRs, to make them go away. (I'll do that here in a minute.) I hope the infrastructure issue will get fixed soon. It's not the first time that something like this happens here on GitHub. |
a3ff673
to
ef01e17
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - looks good to me.
@rwgk the failure appears to be a timeout if you could restart please. |
Description
Implements the suggestion from #5407 (comment)
Not sure if this is the best way.