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

Register BTree/Bucket as MutableMapping and Set/TreeSet as MutableSet, add missing methods/operators #164

Merged
merged 3 commits into from
Apr 10, 2021

Conversation

jamadden
Copy link
Member

@jamadden jamadden commented Apr 8, 2021

Add tests to verify they implement the required methods. Turns out they didn't, so implement them.
This means popitem for the mappings, and pop/discard/isdisjoint plus a swath of operators for the sets.

Do this in both C and Python. The implementations are probably on the naive side, in that they work with generic iterators and don't try to do any complex structural merging, but that makes them easier to understand and hopefully be correct. Performance shouldn't be too bad, nothing worse than O(N*logM) (roughly). The __iand__ method requires extra storage space of up to O(N), and __xor__ requires storage of O(N*M).

Fixes #121

This was a lot bigger than I expected.

@jamadden jamadden requested a review from icemac April 8, 2021 20:03
Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

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

LGTM for the Python code. I cannot judge the C code.

src/BTrees/_base.py Outdated Show resolved Hide resolved
@jamadden
Copy link
Member Author

jamadden commented Apr 9, 2021

LGTM for the Python code. I cannot judge the C code.

I firmly admit to not being a C API expert. I'm at best a journeyman, and I am responsible for at least one true memory leak in zope.interface. That leak involved garbage collection, not regular methods.

Here, I've done my best to keep the C implementations simple, and they don't involve type-level GC changes, so I hope that's a point in favor of this PR :) Also, these are mostly new methods and operators, so even if there are some memory leaks, they won't be sudden and widespread — hopefully people can help report them.

I think I can mostly rule out obvious memory misuse errors, as they would result in segfaults; I know, I experienced them several times during development.

As always, more eyeballs are welcome, so I will wait at least several hours before merging (it's after my bedtime in my timezone anyway).

Thank you!

Add tests to verify they implement the required methods. Turns out they didn't, so implement them.
This means popitem for the mappings, and pop/discard/isdisjoint plus a swath of operators for the sets.

Do this in both C and Python. The implementations are probably on the naive side, in that they work with generic iterators and don't try to do any complex structural merging,
but that makes them easier to understand and hopefully be correct. Performance shouldn't be too bad, nothing worse than O(N*logM) (roughly). One method requires extra storage space of O(N), and __xor__ requires storage of O(N*M).

Fixes #121
@jamadden jamadden merged commit 02bcfde into master Apr 10, 2021
@jamadden jamadden deleted the issue121 branch April 10, 2021 12:44
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.

BTrees should register as collections.abc.MutableMapping and MutableSet
2 participants