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

Allow generics in derive(ByteEq, ByteHash). #219

Merged
merged 1 commit into from
May 28, 2024

Conversation

zachs18
Copy link
Contributor

@zachs18 zachs18 commented Jan 6, 2024

Closes #217

Note that the ergonomics of derive(ByteEq, ByteHash) on a generic type are still not great, since NoUninit (or Pod) is required but cannot be derived (note the manual impl in the added doctests), but derive(NoUninit) explicitly doesn't support generics and gives a better error message, instead of silently ignoring generics like derive(ByteEq, ByteHash) used to, so this is strictly an improvement.

Note that the ergonomics are still not great, since `NoUninit` is also required to use `derive(ByteEq, ByteHash)`, and `derive(NoUninit)` doesn't support generics,
but that *explicitly* doesn't support generics, instead of silently ignoring generics like `derive(ByteEq, ByteHash)` used to, so this is strictly an improvement.
@Lokathor Lokathor added semver-minor semver minor change semver-derive We need to update the main crate's use of the derive crate labels Jan 6, 2024
@Lokathor Lokathor requested a review from fu5ha January 25, 2024 00:33
@Lokathor
Copy link
Owner

Passing this review off to @fu5ha , time permitting

@Lokathor Lokathor merged commit 9a27279 into Lokathor:main May 28, 2024
14 checks passed
zachs18 added a commit to zachs18/bytemuck that referenced this pull request Jun 6, 2024
Note that the ergonomics are still not great, since `NoUninit` is also required to use `derive(ByteEq, ByteHash)`, and `derive(NoUninit)` doesn't support generics,
but that *explicitly* doesn't support generics, instead of silently ignoring generics like `derive(ByteEq, ByteHash)` used to, so this is strictly an improvement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-derive We need to update the main crate's use of the derive crate semver-minor semver minor change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential rustc bug related to deriving ByteEq
2 participants