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

Cast member pointer arguments of class_<T>::def() to member-of-T #471

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

oremanj
Copy link
Contributor

@oremanj oremanj commented Mar 12, 2024

Fixes #470; see that issue for a more detailed explanation of the problem. This ports over pybind's solution with some C++ modernization.

Copy link
Owner

@wjakob wjakob left a comment

Choose a reason for hiding this comment

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

I have been negative about this particular change in the past because I felt it adds too much boilerplate + templates that have to be instantiated for every function binding. I am not totally opposed but mainly interested in finding a simple and efficient solution.

include/nanobind/nb_class.h Outdated Show resolved Hide resolved
include/nanobind/nb_class.h Outdated Show resolved Hide resolved
@oremanj
Copy link
Contributor Author

oremanj commented Mar 12, 2024

Thanks for encouraging me to simplify this. Please take another look; I think you'll be much happier with the second version. It should work on anything that cpp_function does, because it's implemented inside the relevant overloads of cpp_function.

@wjakob
Copy link
Owner

wjakob commented Mar 12, 2024

Yes, I am much happier with this version, thank you for making the changes. This looks good to merge to me, shall I?

@oremanj
Copy link
Contributor Author

oremanj commented Mar 12, 2024

Go for it - thank you!

@wjakob
Copy link
Owner

wjakob commented Mar 12, 2024

Sorry, one more request: could you add a changelog entry?

@wjakob
Copy link
Owner

wjakob commented Mar 12, 2024

I think that the issue in #284 is also simplified by this commit.

@oremanj
Copy link
Contributor Author

oremanj commented Mar 12, 2024

Added changelog entry.

@wjakob wjakob merged commit 754e1b2 into wjakob:master Mar 12, 2024
24 checks passed
@wjakob
Copy link
Owner

wjakob commented Mar 12, 2024

Thank you 👍

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.

[BUG]: can't call exposed member of a type not known to nanobind
2 participants