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

Implement Arbitrary for arrays #226

Closed
wants to merge 2 commits into from
Closed

Conversation

kurnevsky
Copy link
Contributor

@kurnevsky kurnevsky commented Jan 10, 2019

It increases compilation time from ~3.5s to 13.5s in release mode on my laptop. Also not sure how long shrinking can take for [T; 32]. What do you think?

Fixes #92 #187

@BurntSushi
Copy link
Owner

I think my previous opinion hasn't really changed, and a ~4x hit to compile times seemingly confirms it.

@kurnevsky
Copy link
Contributor Author

@BurntSushi I think it's essential to have this implementation to compete with proptest. If compilation time is so important we can have it behind a feature gate (added with the last commit).

@BurntSushi
Copy link
Owner

I don't agree. We don't need to "compete" with anyone. If proptest is better for arrays, then use proptest.

@jturner314
Copy link
Contributor

I came across this PR because I wanted this feature and had to resort to using tuples and then manually converting them to arrays in the body of the function. (My use case for this is randomizing the shape of an n-dimensional array where n=8.)

I understand the issue with compile times, and I ran cargo clean; time cargo build for a few cases on my machine (Core i5-2520M):

  • without arrays feature: 24.185s
  • with arrays feature: 40.015s
  • with arrays feature, but limited to arrays with length <= 8 instead of 32: 24.587s

Note that these times include building the dependencies; I'm not sure how to measure the time to build quickcheck by itself. I think it makes sense to include the time to build the dependencies anyway, though, because in practice users only need to build quickcheck and its dependencies once; they don't need to rebuild quickcheck each time they test their crate.

The additional build time to support arrays of length <= 8 is negligible. What I'd suggest is:

  • Add support by default for arrays of length <= 8.
  • If users want support for longer arrays, put that behind a feature flag (long_arrays or something).

Thoughts?

@BurntSushi
Copy link
Owner

We've made it this far without Arbitrary impls for arrays, we can wait a bit longer for const generics to land.

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.

Add Arbitrary implementation for small arrays
3 participants