Skip to content

Support frozenset, tuple as dict keys (alternative approach) #3887

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

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

ecatmur
Copy link
Contributor

@ecatmur ecatmur commented Apr 20, 2022

Description

Alternative to #3886
Inspired by #3836

Add frozenset as a pybind11 type.
Add freeze() function converting set to frozenset and list to tuple; use
it in std::set and std::map casters.
Add tests.

Suggested changelog entry:

When used as keys (e.g. in std::map), sequence and set types now convert to tuple and frozenset (instead of unhashable list and set).

pybind#3836

Add frozenset as a pybind11 type.
Add freeze() function converting set to frozenset and list to tuple; use
it in std::set and std::map casters.
Add tests.
@Skylion007
Copy link
Collaborator

I am wondering though if there is anyway we can allow users to extend the 'freeze' function for custom Python types, perhaps using template specialization tricks? Thoughts @rwgk @henryiii?

@Skylion007
Copy link
Collaborator

Hmm, also I noticed the freeze PEP was rejected as bugprone. Really this is a symptom of us needing to have better specialization and user configuration of key / value converters. Related to the issue we have found in #3838

ecatmur and others added 12 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
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.
@rwgk
Copy link
Collaborator

rwgk commented May 16, 2022

Hi @ecatmur is this ready for re-review? (Or are you still working on it?)

@ecatmur
Copy link
Contributor Author

ecatmur commented May 27, 2022

Hi @ecatmur is this ready for re-review? (Or are you still working on it?)

Sorry, yeah. I was exploring some alternate approaches (I don't particularly like frozen_type_name) but nothing worked out any better.

@leslinice
Copy link

leslinice commented May 27, 2022 via email

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