-
Notifications
You must be signed in to change notification settings - Fork 7
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 py::nullptr_default_arg
(intended for PyObject *
arguments)
#30081
Conversation
…52272643d073152e7f3acdd
py::nullptr_default_arg
py::nullptr_default_arg
(intended for PyObject *
arguments)
include/pybind11/attr.h
Outdated
handle value; ///< Associated Python object | ||
const char *name; ///< Argument name | ||
const char *descr; ///< Human-readable version of the argument value | ||
handle value; ///< Associated Python object |
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.
Should this be made to a std::optional rather than adding a boolean? Using an optional would enforce that no value is hold when value_is_nullptr is True.
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 agree, but
- that would be a much more intrusive change,
- especially also because we still care (***) about C++11 compatibility.
std::optional
requires C++17.
(***) Because I'm still keeping pywrapcc very closely aligned with upstream pybind11 master.
Net change:
I added a comment to explain why value_is_nullptr
exists (safety).
@@ -127,4 +127,26 @@ TEST_SUBMODULE(type_caster_pyobject_ptr, m) { | |||
(void) py::cast(*ptr); | |||
} | |||
#endif | |||
|
|||
m.def("pyobject_ptr_from_handle_nullptr", []() { |
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.
not very clear at first glance how this handle_nullptr logic is used with the rest of the change. Could you elaborate a bit more on this? Also surprising to me is that if a py::handle can already hold a nullptr, then why do we need to add another value_is_nullptr member to the arg struct, which already hold a py::handle member?
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.
Ah, yes, the information you're looking for was only in a commit message before (pybind11 master PR used to start this work, but closed already):
I added a comment to explain.
The core idea is to cleanly exercise a critical sub-feature in isolation. It did not need production code changes, but I don't think it is explicitly exercised anywhere else already.
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.
Thanks!
include/pybind11/attr.h
Outdated
@@ -179,13 +179,20 @@ struct argument_record { | |||
const char *name; ///< Argument name | |||
const char *descr; ///< Human-readable version of the argument value | |||
handle value; ///< Associated Python object | |||
// The explicit value_is_nullptr variable is for safety, to unambiguously | |||
// distinguish these two cases in all situations: | |||
// * value is nullptr on purpose. |
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.
Which of the case does True correspond to?
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.
Good point, thanks. I made the comment more precise.
Note for completeness: I meant to squash-merge but forgot to change the Merge button before clicking on it. |
The `py::nullptr_default_arg` feature was added with google/pybind11clif#30081 Addresses this use case: * google3/learning/deepmind/science/protein_platform/structure/python/string_array_utils.h;l=12;rcl=491437490 PiperOrigin-RevId: 589268503
Description
Enables distinguishing
None
and "no argument passed" / "default argument" forPyObject *
arguments.Example usage:
Notes:
In contrast to the above,
py::arg("ptr") = nullptr
is equivalent to passingNone
.For any
T*
other thanPyObject*
, the equivalence ofNone
andnullptr
is fine. It's only forPyObject*
that the distinction matters.In manually written code the
py::nullptr_default_arg
behavior could also be achieved with overloads, but the approach as shown in the example is an easier target for a code generator (PyCLIF), and scales better in general.Suggested changelog entry: