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 unsigned variants #122

Merged
merged 3 commits into from
Mar 17, 2020
Merged

Add unsigned variants #122

merged 3 commits into from
Mar 17, 2020

Conversation

jamadden
Copy link
Member

@jamadden jamadden commented Feb 14, 2020

Fixes #120

I went ahead and just added unsigned versions of all the datastructures because…why not, it was easier that way (CLI scripting FTW).

This wasn't hard, just tedious, and there's an unfortunate amount of copy-n-paste at the Python level, so I may have missed something. (It would be nice to be able to reduce that, especially because I'd like to see about adding some more memory-efficient types, too, such as byte strings (char*))

I know some updated documentation is still owed.

One thing that is a bit weird, and deserves documentation at least, is the handling of the "weight" arguments to the intersection functions. You are allowed to pass negative values to unsigned datastructures, but the results you get will be meaningless. Python doesn't have a standard way to handle over/underflow in arguments. I figured those are lightly used and unlikely to be much of a problem, but I'm sure we could figure out a solution if necessary.

This has nothing to do with this PR, I just happened to notice it. Another thing that's a bit weird is that we talk about "family32" and "family64" expecting them to be 32- and 64-bit datatypes, respectively, but then the C code goes and uses standard C int and long long, which is not technically the same thing. The Python implementation isn't really any better, relying on the native sizes of the struct module. But we have specific tests at the limits of 32- and 64-bit types, so we rely on those sizes matching up. We'd be broken on a platform where they didn't.

@freddrake
Copy link
Contributor

I'll try to take a look over the weekend.

@jamadden
Copy link
Member Author

Looking at the coverage numbers, I see files like test_LLBTree.py (that wasn't changed here) have low coverage numbers. This looks to be because of issues with the test_suite functions methods repeating or skipping test names.

I'd like to see about removing those functions.

@jamadden
Copy link
Member Author

I think that's all I want to do in this PR. It's big enough as it is :) I'd still like to address the duplication that made this tedious, but that's a followup PR.

@icemac
Copy link
Member

icemac commented Mar 2, 2020

Anyone who has the steam and knowledge to review this PR?

@jamadden
Copy link
Member Author

jamadden commented Mar 5, 2020

I would appreciate any reviews and help getting this (and #123) merged.

@mgedmin
Copy link
Member

mgedmin commented Mar 5, 2020

big diff is big :(

test__UUBTree.py is complete and passing on Python 3
And some other small tweaks to the documentation to make cross-references work and things generally look good.
@jamadden
Copy link
Member Author

jamadden commented Mar 5, 2020

I've rebased and reworked to reduce the diff as much as possible. There's still a lot of boilerplate files, though (hence #123 :) )

Unfortunately, today's release of persistent is triggering what looks like a virtualenv bug under PyPy: The headers aren't getting installed to the correct virtualenv place; instead they try to go to the system header directory, which isn't writable.

@freddrake
Copy link
Contributor

I'm afraid my weekends have been too busy lately, and that'll continue for another month. Will try to squeeze it in anyway... but we'll see.

@jamadden
Copy link
Member Author

Are there any objections if I go ahead and merge and release these to get them out there?

@jamadden jamadden merged commit 3cbf0dc into master Mar 17, 2020
@jamadden jamadden deleted the issue120 branch March 17, 2020 14:14
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.

Support for (64-bit) unsigned keys/values.
4 participants