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

Add py::nullptr_default_arg (intended for PyObject * arguments) #30081

Merged
merged 6 commits into from
Dec 8, 2023

Conversation

rwgk
Copy link
Contributor

@rwgk rwgk commented Dec 2, 2023

Description

Enables distinguishing None and "no argument passed" / "default argument" for PyObject * arguments.

Example usage:

    m.def(
        "pyobject_nullptr_default_arg_example",
        [](PyObject *ptr) {
            if (ptr == nullptr) {
                return py::str("ptr == nullptr");
            }
            return py::repr(ptr);
        },
        py::arg("ptr") = py::nullptr_default_arg());

Notes:

  • In contrast to the above, py::arg("ptr") = nullptr is equivalent to passing None.

  • For any T* other than PyObject*, the equivalence of None and nullptr is fine. It's only for PyObject* 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:

@rwgk rwgk changed the title Add py::nullptr_default_arg Add py::nullptr_default_arg (intended for PyObject * arguments) Dec 2, 2023
@rwgk rwgk marked this pull request as ready for review December 5, 2023 15:37
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

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.

Copy link
Contributor Author

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", []() {

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?

Copy link
Contributor Author

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):

pybind/pybind11@a3fd84d

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.

Choose a reason for hiding this comment

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

Thanks!

@@ -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.

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?

Copy link
Contributor Author

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.

@rwgk rwgk merged commit a2b26e1 into google:main Dec 8, 2023
147 checks passed
@rwgk rwgk deleted the py_nullptr_default_arg_pywrapcc branch December 8, 2023 21:14
@rwgk
Copy link
Contributor Author

rwgk commented Dec 8, 2023

Note for completeness: I meant to squash-merge but forgot to change the Merge button before clicking on it.

rwgk pushed a commit to google/clif that referenced this pull request Dec 11, 2023
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
@rwgk rwgk added the could be backported Could be backported to pybind11 master label Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
could be backported Could be backported to pybind11 master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants