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

WIP/POC: num-bigint support #235

Closed
wants to merge 3 commits into from

Conversation

clarfonthey
Copy link

@clarfonthey clarfonthey commented Mar 18, 2021

Right now, the implementation feels disgusting because I mostly just added clones and amperstands wherever they were needed, but this is mostly just a proof-of-concept because I was looking for an implementation myself and noticed that someone else (see: #222) also wanted one.

Honestly, going forward, I think that there are far too many macros in use here and it would make a whole lot more sense to generalise the code with macros, so that we can just auto-magically implement these for as many types as we want. But, that would require a breaking change.

I don't think that this should affect the runtime of the primitive implementations but will want to benchmark that as well.

No idea how quickly I'll be able to work on this since I don't have a lot of time/energy to put into it, but figured I'd at least start out by opening a PR and seeing what people think, offer suggestions, etc.


Current changes:

  1. Adds BigInt and BigUint submodules of proptest/num, for now.
  2. Factors out separate uint and int submodules of proptest/num which include a generic BinaryTree. For now, this still re-exports wrappers to the individual type submodules to maintain compatibility. Ideally, in the future, we'd just get rid of these copies in favour of the generic versions.
  3. Updates the BinaryTree implementation to include an extra "scratch" field which can be used to save old memory allocations for bigint implementations.

@clarfonthey
Copy link
Author

clarfonthey commented Apr 3, 2021

I think this is at a point where it's an improvement over the existing implementation, but would love a second opinion from @Centril or @AltSysrq since you'd be the ones merging. Not 100% sure where we want to export the generic version, or what to call it.

There's also the potential for this to be slower for native integers, but I'm not sure if it's at a point where it matters. Optimisations are usually disabled during tests and so we can't rely on that.

@cameron1024
Copy link
Member

Thanks for the PR

My initial thought is that I'm not sure that bigint support should live in proptest, rather than in num-bigint. I view proptest's role here a bit closer to something like serde - crates that define new types would typically implement Serialize and Deserialize for their type, gated behind a serde feature flag, rather than the other way round (i.e. there's no num-bigint flag in serde).

In fact, num-bigint already has quickcheck and arbitrary feature flags, so it looks like there's already some history there. It's unfortunate we can't reuse those implementations very easily :/

Slowing down existing int types also seems like something we should ideally avoid, though I haven't looked at the code, so not sure exactly what sort of impact we're talking about. FWIW, I fairly often run proptest with --release, since I have quite a lot of long-running tests.

That said, keen to hear other people's points of view on this issue. There may be details about num-bigint that I'm not aware of that mean the impl really should live here 🤷 Happy to be corrected on this

@clarfonthey
Copy link
Author

So, my thought process here is that the logic for generating bigints is the same across multiple crates, and I just used num-bigint since it's a common type, but it could in theory be used across crates for other types.

How would you feel instead if this generated slices of ints instead of bigints? That way it could be converted to other types, but would also not be specific to the num-bigint crate. As it stands, the main reason why I made this is because it would be a lot easier to make with access to the internals in this crate, rather than from the ground up in other crates.

@clarfonthey
Copy link
Author

Going to close this since it's years old and I'm not particularly interested in resolving the conflicts. If someone else wants to take this on, please feel free to reuse my work.

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.

None yet

2 participants