Skip to content

Conversation

@gentlegiantJGC
Copy link
Contributor

@gentlegiantJGC gentlegiantJGC commented Feb 18, 2025

Description

The def_property family blindly ignore the keep_alive and call_guard arguments passed to them making them confusing to use.
This adds a static_assert if either is passed to make it clear it doesn't work.
I would prefer this to be a compiler warning but I can't find a way to do that. Is that even possible?

This does not fix - #4236 or #5046 but makes it clear that they don't work.

Suggested changelog entry:

Stop keep_alive and call_guard being passed to def_property functions.

gentlegiantJGC and others added 2 commits February 18, 2025 12:27
The def_property family blindly ignore the keep_alive and call_guard arguments passed to them making them confusing to use.
This adds a static_assert if either is passed to make it clear it doesn't work.
I would prefer this to be a compiler warning but I can't find a way to do that. Is that even possible?
@rwgk
Copy link
Collaborator

rwgk commented Feb 26, 2025

(random timing, I'm cleaning up my email backlog)

Looks good, thanks! — 'll get back here a few days after #5542 is merged, but before the v3.0.0 release.

@gentlegiantJGC
Copy link
Contributor Author

Looks like this got missed

@rwgk
Copy link
Collaborator

rwgk commented Nov 14, 2025

Oh, sorry. Could you please update this branch and tag me if you see that the CI passed?

@gentlegiantJGC
Copy link
Contributor Author

I think the android runner is broken. This is also an issue in #5896

@rwgk
Copy link
Collaborator

rwgk commented Nov 15, 2025

Ignoring, after two unsuccessful reruns:

CIBW / Android wheel ubuntu-latest (pull_request) (pull_request)Failing after 10m

@rwgk rwgk merged commit af796d0 into pybind:master Nov 15, 2025
83 of 85 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Nov 15, 2025
@gentlegiantJGC gentlegiantJGC deleted the property-keep-alive branch November 17, 2025 08:14
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Jan 21, 2026
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.

2 participants