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 anyset & frozenset, enable copying (cast) to std::set #3901

Merged
merged 9 commits into from
May 5, 2022

Conversation

ecatmur
Copy link
Contributor

@ecatmur ecatmur commented Apr 24, 2022

Description

Add anyset & frozenset, enable copying (cast) to std::set.

For the reverse direction, std::set still casts to set. This is in concordance with the behavior for sequence containers, where e.g. tuple casts to std::vector, but std::vector casts to list.

Extracted from #3886.

Suggested changelog entry:

``anyset`` & ``frozenset`` were added, with copying (cast) to ``std::set`` (similar to ``set``).

ecatmur added 2 commits April 24, 2022 21:54
For the reverse direction, std::set still casts to set. This is in concordance with the behavior for sequence containers, where e.g. tuple casts to std::vector but std::vector casts to list.

Extracted from pybind#3886.
since this will be part of pybind11 public API
@rwgk rwgk changed the title Add frozenset, and allow it cast to std::set Add any_set & frozenset, enable copying (cast) to std::set Apr 25, 2022
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.

Looks great, thanks!

include/pybind11/pytypes.h Outdated Show resolved Hide resolved
include/pybind11/pytypes.h Show resolved Hide resolved
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.

This just crossed my mind: could you please also add minimal coverage for the frozenset C++ API (constructors, size, empty, contains)?

Copy link
Collaborator

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

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

Update: NVM, misread the code. Agree with @rwgk about additional tests would be nice.

@Skylion007
Copy link
Collaborator

C-API docs unfortunately seem to indicate it should be "anyset" instead of "any_set" to be consistent: https://docs.python.org/3/c-api/set.html#c.PySet_Contains

Copy link
Collaborator

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

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

Actually, looking over it I do have some nits before merging.

include/pybind11/pytypes.h Outdated Show resolved Hide resolved
@rwgk
Copy link
Collaborator

rwgk commented Apr 25, 2022

C-API docs unfortunately seem to indicate it should be "anyset" instead of "any_set" to be consistent: https://docs.python.org/3/c-api/set.html#c.PySet_Contains

I don't have a strong opinion about this one, but usually I try to emulate what I see elsewhere, anyset in this case would be my choice.

Regarding the ctor questions, not sure, usually I figure this out by writing unit tests fully covering the public API, then it's easy to see/decide what is useful and practical.

ecatmur and others added 4 commits May 1, 2022 12:17
and rename anyset methods
Making frozenset non-default constructible means that we need to adjust pyobject_caster to not require that its value is default constructible, by initializing value to a nil handle.  This also allows writing C++ functions taking anyset, and is arguably a performance improvement, since there is no need to allocate an object that will just be replaced by load.

Add some more tests, including anyset::empty, anyset::size, set::add and set::clear.
@ecatmur
Copy link
Contributor Author

ecatmur commented May 1, 2022

Hope I've addressed all concerns.

Note the change to pyobject_caster (to default-initialize with value a nil handle) - this should be very slightly more efficient and still safe, since load always assigns to value (so previously it was being allocated to no purpose). This is necessitated by anyset and frozenset not being default constructible.

@ecatmur ecatmur changed the title Add any_set & frozenset, enable copying (cast) to std::set Add anyset & frozenset, enable copying (cast) to std::set May 1, 2022
template <typename T = type, enable_if_t<std::is_same<T, handle>::value, int> = 0>
pyobject_caster() : value() {}

template <typename T = type, enable_if_t<std::is_base_of<object, T>::value, int> = 0>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure about the changes on the caster: So without a default ctor, this ctor is needed on pyobject_caster? At the very least, we should have a comment about why there are necessary. I am also worried that there are edge cases where this might break when it is used in downstream applications.

Thoughts @rwgk @henryiii

Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC we have a choice:

  • Either the change here.
  • Or we need to make anyset and frozenset default constructible.

#3901 (comment)

I agree but think it is not just "still safe" but actually safer: if the previously default-constructed object is accidentally used the symptoms could be highly confusing (silent failures). A noisy failure (segfault b/o nullptr) seems better.

I'll run our (Google's) global testing with this PR to see if there are breakages and report back here. I might have the results only tomorrow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No breakages! This PR is great. If anyone was previously relying on the default-constructed value, they will probably be glad to see that exposed as an obvious bug now.

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.

I tried to enhance the PR description. Please review & correct as needed.

@@ -1784,25 +1784,37 @@ class kwargs : public dict {
PYBIND11_OBJECT_DEFAULT(kwargs, dict, PyDict_Check)
};

class set : public object {
class anyset : public object {
protected:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, last concern. Why is the scope here protected? I don't see any other Pytypes doing that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Specifically, I am worried it might interfere with the static "check_" method that isinstance is suppose to be using.

return T::check_(obj);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good question, it turns out the protected: here doesn't do anything, because the PYBIND11_OBJECT macro expands to starting with public:. See clang -E output below.

(Note that there is an isinstance<anyset> in stl.h, i.e. this had to be the case somehow.)

I think it's best to not get fancy here and to simply remove the protected: line.

class anyset : public object {
protected:
    public: [[deprecated("Use reinterpret_borrow<" "anyset" ">() or reinterpret_steal<" "anyset" ">()")]] anyset(handle h, bool is_borrowed) : object(is_borrowed ? object(h, borrowed_t{}) : object(h, stolen_t{})) {} anyset(handle h, borrowed_t) : object(h, borrowed_t{}) {} anyset(handle h, stolen_t) : object(h, stolen_t{}) {} [[deprecated("Use py::isinstance<py::python_type>(obj) instead")]] bool check() const { return m_ptr != nullptr && ((_Py_IS_TYPE(((const PyObject*)(m_ptr)), &PySet_Type) || _Py_IS_TYPE(((const PyObject*)(m_ptr)), &PyFrozenSet_Type) || PyType_IsSubtype((((PyObject*)(m_ptr))->ob_type), &PySet_Type) || PyType_IsSubtype((((PyObject*)(m_ptr))->ob_type), &PyFrozenSet_Type)) != 0); } static bool check_(handle h) { return h.ptr() != nullptr && (_Py_IS_TYPE(((const PyObject*)(h.ptr())), &PySet_Type) || _Py_IS_TYPE(((const PyObject*)(h.ptr())), &PyFrozenSet_Type) || PyType_IsSubtype((((PyObject*)(h.ptr()))->ob_type), &PySet_Type) || PyType_IsSubtype((((PyObject*)(h.ptr()))->ob_type), &PyFrozenSet_Type)); } template <typename Policy_> anyset(const ::pybind11::detail::accessor<Policy_> &a) : anyset(object(a)) {} anyset(const object &o) : object(o) { if (m_ptr && !check_(m_ptr)) throw ::pybind11::type_error("Object of type '" + ::pybind11::detail::get_fully_qualified_tp_name((((PyObject*)(m_ptr))->ob_type)) + "' is not an instance of '" "anyset" "'"); } anyset(object &&o) : object(std::move(o)) { if (m_ptr && !check_(m_ptr)) throw ::pybind11::type_error("Object of type '" + ::pybind11::detail::get_fully_qualified_tp_name((((PyObject*)(m_ptr))->ob_type)) + "' is not an instance of '" "anyset" "'"); }

public:
    size_t size() const { return static_cast<size_t>(PySet_Size(m_ptr)); }
    bool empty() const { return size() == 0; }
    template <typename T>
    bool contains(T &&val) const {
        return PySet_Contains(m_ptr, detail::object_or_cast(std::forward<T>(val)).ptr()) == 1;
    }
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, yes. Removed.

@rwgk rwgk merged commit 68a0b2d into pybind:master May 5, 2022
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label May 5, 2022
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 7, 2022
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.

4 participants