-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: null pointer dereference in attr_with_type_hint
#5576
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: null pointer dereference in attr_with_type_hint
#5576
Conversation
Signed-off-by: Michael Carlstrom <[email protected]>
@timohl looks good to me (the suggestions I posted are pretty minor), could you please take a quick look, too? |
I believe theres still a dereference issue since the function record is also set to a pybind11/include/pybind11/pybind11.h Line 168 in 97022f8
I guess the correct behaviour would be a pybind11 failure? |
Thanks @kvarg for pointing this out, I totally overlooked that Ideally, for each additional guard we're adding, we should have a corresponding unit test. |
I got it to have the same error as using m.attr() without creating bindings. "AttributeError: 'module' object has no attribute 'foo4' |
Currently doing the following results in the following. struct foo4 {};
m.attr_with_type_hint<foo4>("foo4") = 3; It makes the type hint |
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine as a fix for a regression.
But I realize now, it's generally not a great pattern to create a dummy object (the function record) to reuse a complex function. Careful factoring would be better, so that generate_type_signature<T>()
does not involve a function record at all.
Signed-off-by: Michael Carlstrom <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed a tiny typo: 07c8453
Waiting for GitHub Actions to finish, then I'll merge.
attr_with_type_hint
attr_with_type_hint
Description
Fixes regression caught in #5575
Suggested changelog entry: