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.

Sign up for GitHub

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

Jump to bottom

Use std::unique_ptr in pybind11_getbuffer #5435

Merged
merged 2 commits into from
Nov 8, 2024
Merged

Use std::unique_ptr in pybind11_getbuffer #5435

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7fcf820
Use std::unique_ptr in pybind11_getbuffer
QuLogic Nov 7, 2024
ef01e17
Move final assignment later
QuLogic Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 7 additions & 10 deletions include/pybind11/detail/class.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,9 +583,9 @@ extern "C" inline int pybind11_getbuffer(PyObject *obj, Py_buffer *view, int fla
return -1;
}
std::memset(view, 0, sizeof(Py_buffer));
buffer_info *info = nullptr;
std::unique_ptr<buffer_info> info = nullptr;
try {
info = tinfo->get_buffer(obj, tinfo->get_buffer_data);
info.reset(tinfo->get_buffer(obj, tinfo->get_buffer_data));
} catch (...) {
try_translate_exceptions();
raise_from(PyExc_BufferError, "Error getting buffer");
Expand All @@ -596,17 +596,13 @@ extern "C" inline int pybind11_getbuffer(PyObject *obj, Py_buffer *view, int fla
}

if ((flags & PyBUF_WRITABLE) == PyBUF_WRITABLE && info->readonly) {
delete info;
// view->obj = nullptr; // Was just memset to 0, so not necessary
set_error(PyExc_BufferError, "Writable buffer requested for readonly storage");
return -1;
}

// 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;
Copy link

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.

view->buf = info->ptr;
view->itemsize = info->itemsize;
view->len = view->itemsize;
for (auto s : info->shape) {
Expand All @@ -624,23 +620,20 @@ extern "C" inline int pybind11_getbuffer(PyObject *obj, Py_buffer *view, int fla
if ((flags & PyBUF_C_CONTIGUOUS) == PyBUF_C_CONTIGUOUS) {
if (PyBuffer_IsContiguous(view, 'C') == 0) {
std::memset(view, 0, sizeof(Py_buffer));
delete info;
set_error(PyExc_BufferError,
"C-contiguous buffer requested for discontiguous storage");
return -1;
}
} else if ((flags & PyBUF_F_CONTIGUOUS) == PyBUF_F_CONTIGUOUS) {
if (PyBuffer_IsContiguous(view, 'F') == 0) {
std::memset(view, 0, sizeof(Py_buffer));
delete info;
set_error(PyExc_BufferError,
"Fortran-contiguous buffer requested for discontiguous storage");
return -1;
}
} else if ((flags & PyBUF_ANY_CONTIGUOUS) == PyBUF_ANY_CONTIGUOUS) {
if (PyBuffer_IsContiguous(view, 'A') == 0) {
std::memset(view, 0, sizeof(Py_buffer));
delete info;
set_error(PyExc_BufferError, "Contiguous buffer requested for discontiguous storage");
return -1;
}
Expand All @@ -650,7 +643,6 @@ extern "C" inline int pybind11_getbuffer(PyObject *obj, Py_buffer *view, int fla
// https://docs.python.org/3/c-api/buffer.html#contiguity-requests
if (PyBuffer_IsContiguous(view, 'C') == 0) {
std::memset(view, 0, sizeof(Py_buffer));
delete info;
set_error(PyExc_BufferError,
"C-contiguous buffer requested for discontiguous storage");
return -1;
Expand All @@ -665,6 +657,11 @@ extern "C" inline int pybind11_getbuffer(PyObject *obj, Py_buffer *view, int fla
}
}

// Set these after all checks so they don't leak out into the caller, and can be automatically
// cleaned up on error.
view->buf = info->ptr;
view->internal = info.release();
view->obj = obj;
Py_INCREF(view->obj);
return 0;
}
Expand Down
Loading