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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Const generic arbitrary #288

Merged
merged 3 commits into from
Jan 14, 2023
Merged

Const generic arbitrary #288

merged 3 commits into from
Jan 14, 2023

Conversation

cameron1024
Copy link
Member

In the previous ticket, I added Strategy impls for const generic arrays of strategies, but forgot about Arbitrary 馃う

This one adds Arbitrary impls for const generic arrays of types which impl Arbitrary.

As with the previous PR, I don't think this is a breaking change.

Copy link
Collaborator

@rex-remind101 rex-remind101 left a comment

Choose a reason for hiding this comment

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

just to make sure i'm understanding, this change makes it so that Arbitrary is inherited across the board for const generic arrays so the user doesn't need to use a macro?

if so seems good to me. one thing i've noticed though is the macro arbitrary! seems to be used for these declarations internally instead. however, it seems pretty simple to just inherit the trait directly like you have here so i don't have a problem with that.

@cameron1024
Copy link
Member Author

cameron1024 commented Jan 7, 2023

I don't think that's quite right:

The array macro only serves to write boilerplate impls.

It's roughly equivalent to :

impl<A: Arbitrary> Arbitrary for [A; 1] { ... }
impl<A: Arbitrary> Arbitrary for [A; 2] { ... }
// ...
impl<A: Arbitrary> Arbitrary for [A; 32] { ... }

except with less typing. array! was never exposed to users. There wasn't a macro they could use. Instead, they just couldn't have Arbitrary impls for arrays longer than 32 elements

The new code uses const generics to provide impls for [A; N], regardless the value of N.

Another way of thinking about it is that the previous code was a bit like this hypothetical syntax:

impl<A: Arbitrary, const N: usize> Arbitrary for [A; N] where A >= 1, A <= 32 {}

and this PR just removes that where clause. Of course this isn't supported at the language level, but conceptually we're just relaxing a constraint, so it shouldn't break compat

As for arbitrary!, I haven't looked at it in great detail, but I generally find looking at decl-macros that I didn't write pretty tricky, and avoid them where possible. IMO, this impl is pretty self-describing and readable

@rex-remind101
Copy link
Collaborator

Thanks for the explanation, that all makes good sense to me :)

Copy link
Member

@matthew-russo matthew-russo left a comment

Choose a reason for hiding this comment

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

looks good to me

@cameron1024 cameron1024 merged commit 00f29af into master Jan 14, 2023
@cameron1024 cameron1024 deleted the const-generic-arbitrary branch January 14, 2023 21:00
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

3 participants