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

Add list.clear() in the C++ API. #30120

Closed
wants to merge 1 commit into from
Closed

Add list.clear() in the C++ API. #30120

wants to merge 1 commit into from

Conversation

TCKnet
Copy link
Contributor

@TCKnet TCKnet commented May 13, 2024

Description

Add list.clear() in the C++ API, this allows resetting a list (e.g. for doing function argument modification and such).
Append() is already present but there is currently no way to alter or remove items from a list.

Suggested changelog entry:

Add pybind11::list::clear() in C++.

Copy link

google-cla bot commented May 13, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@frigus02
Copy link

Adding pybind11::list::clear() sounds good, but the tests are failing. Can you have a look, please?

@TCKnet
Copy link
Contributor Author

TCKnet commented May 18, 2024

It seems now that some integration tests (Windows, MacOS) fail for unrelated reasons... how can I help fixing?

@frigus02
Copy link

Sorry. I've been out sick for the last few days. This PR is already on my todo list, though. Monday is a public holiday for me. I'll try to get to this when I'm back at work on Tuesday.

@frigus02
Copy link

I had a quick look at the tests but can't immediately figure out what's wrong. I don't think fixing those is highest priority right now. So I think we just ignore them.

However, I'd like to wait merging this PR. I would like to catch up with Ralf first. In general we'd like this repo to have a few differences from https://github.com/pybind/pybind11 as possible. Ideally we can contribute all changes back upstream and delete this fork eventually. This PR would be one more thing for us to upstream. This might be fine, but I think I need to understand the consequences a little better before.

Copy link
Contributor

@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.

Could you please transfer this PR to a https://github.com/pybind/pybind11 PR (upstream pybind11)?

It's still very important that we keep the delta between upstream pybind11 master and pybind11clif as small as possible.

@@ -2195,6 +2195,7 @@ class list : public object {
throw error_already_set();
}
}
void clear() { PyList_SetSlice(m_ptr, 0, PyList_Size(m_ptr), nullptr); }
Copy link
Contributor

Choose a reason for hiding this comment

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

The return value of PyList_SetSlice() needs to be checked, and you need to add throw error_already_set(); if it is not 0:

https://docs.python.org/3/c-api/list.html#c.PyList_SetSlice

(I think it's fine/best to omit a similar check for PyList_Size(): from the context we're sure that m_ptr is a Python list object.)

@rwgk
Copy link
Contributor

rwgk commented Jun 7, 2024

This change was merged as pybind/pybind11#5153. I'll merge upstream master here soon. Closing this PR.

@rwgk rwgk closed this Jun 7, 2024
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