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: enable bind_map with using declarations. #4952

Merged
merged 8 commits into from
Nov 29, 2023
Merged

Conversation

AntoinePrv
Copy link
Contributor

Description

Enable using py::bind_map with map-like things that privately inherit from map + using declarations.

Suggested changelog entry:

- Fix ``bind_map`` with ``using`` declarations.

@rwgk
Copy link
Collaborator

rwgk commented Nov 27, 2023

Could you please add a test that does not work without this PR? — That's the only way to ensure that there are no regressions long-term.

See tests/test_stl_binders.cpp and maybe tests/test_stl_binders.py

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.

LGTM but could you please also add minimal tests in test_stl_bind.py? Just enough to be sure the new py::bind_vector and py::bind_map are not accidentally lost in the future.

tests/test_stl_binders.cpp Outdated Show resolved Hide resolved
@AntoinePrv
Copy link
Contributor Author

LGTM but could you please also add minimal tests in test_stl_bind.py? Just enough to be sure the new py::bind_vector and py::bind_map are not accidentally lost in the future.

IMHO this is not necessary. This was purely a compilation fix.

@AntoinePrv AntoinePrv requested a review from rwgk November 28, 2023 16:05
@rwgk
Copy link
Collaborator

rwgk commented Nov 28, 2023

LGTM but could you please also add minimal tests in test_stl_bind.py? Just enough to be sure the new py::bind_vector and py::bind_map are not accidentally lost in the future.

IMHO this is not necessary. This was purely a compilation fix.

That's what I understood.

But there is a chance that the two new lines are accidentally removed when refactoring.

Adding minimal tests (maybe 2 x 2 lines) is very easy and makes it much less likely that something gets lost accidentally.

@AntoinePrv
Copy link
Contributor Author

AntoinePrv commented Nov 29, 2023

I have added a Python test. Thanks for taking the time.

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.

Thanks, looks great!

Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

This seems to be the only obvious fix. Lambdas can be more expensive to compile, but I think that's with capture, which isn't used for these. So I think it's fine.

@rwgk rwgk merged commit a67d786 into pybind:master Nov 29, 2023
84 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Nov 29, 2023
@henryiii henryiii changed the title fix(stl_bind): Enable bind_map with using declarations. fix: Enable bind_map with using declarations. Mar 26, 2024
@henryiii henryiii changed the title fix: Enable bind_map with using declarations. fix: enable bind_map with using declarations. Mar 26, 2024
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Mar 27, 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